amadan

oficina / fichas Public

Create the sign-in tables: no sign-in could ever complete

The deployed app logged `no such table: auth_links` on the first real
email sign-in. BootSchema was `migrate.Merge(Schema)` — fichas' own two
migrations and nothing else. The commit that wired sign-in meant to add
sessions.Schema and auth.Schema, but its edit looked for `BootSchema =
Schema`, the scaffold spelled it `migrate.Merge(Schema)`, the replace
matched nothing, and nothing checked that it had. Every boot log since
said skipped=2, and nobody read that as two where there should be six.

So for eight days neither sign-in path could finish: the email path needs
auth_links, and the keymail path would have failed on return for want of
the sessions table. Nothing caught it because every test either bypassed
auth through AppForTest or stopped at the guard in front of it, and the
one piece of wiring no test exercised was the one that touches the
database.

The fix is one line. The tests are the point:

- TestBootSchema asserts auth_links and sessions exist after a boot.
- TestASignInCompletesEndToEnd builds the app exactly as main.go does —
  App, NewAuth, UseAuth, no bypass — submits an owner's address, captures
  the email, follows the link, and asserts a session reaches the index.
  It also checks the link uses FICHAS_ORIGIN and is single-use.
- Both were run against the original bug reinstated, and both failed.

Also fixed while here: every ?err= code rendered "That did not work — try
again in a moment", including this one, which is advice that could never
succeed. Each code now says what happened and what to do, without
distinguishing an admitted address from a stranger's. And rastrillo's
keymail failure path sends ?force=1&err=keymail so the page can fall back
to email; the page now honours it with a hidden force field.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev a08557dc2b848e1fa9259b41b9a02ba3ae1685a5 parent c218187
4 files changed, +240 −4
  • internal/fichas/handlers.go +32 −2
  • internal/fichas/migrations.go +13 −1
  • internal/fichas/templates/signin.html +2 −1
  • internal/fichastest/signin_flow_test.go +193 −0

CI passed — run details

CI log 
=== vet ===
go vet ./...
=== fmt ===
=== test ===
go test ./...
?   	amadan.net/oficina/fichas/cmd/fichas	[no test files]
ok  	amadan.net/oficina/fichas/internal/fichas	0.374s
?   	amadan.net/oficina/fichas/internal/fichas/icons	[no test files]
ok  	amadan.net/oficina/fichas/internal/fichas/seal	(cached)
ok  	amadan.net/oficina/fichas/internal/fichastest	0.459s
?   	amadan.net/oficina/fichas/scripts/writeestilo	[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/fichas/handlers.go b/internal/fichas/handlers.go
index f68a64f..a31aacd 100644
--- a/internal/fichas/handlers.go
+++ b/internal/fichas/handlers.go
@@ -66,9 +66,14 @@ func (a *app) actor(r *http.Request) (Actor, error) {
// they exist so the router names methods on the app and a nil auth is
// refused in one place rather than panicking in six.
func (a *app) signin(w http.ResponseWriter, r *http.Request) {
+ q := r.URL.Query()
render(w, "signin", map[string]any{
- "Sent": r.URL.Query().Get("sent") == "1",
- "Error": r.URL.Query().Get("err"),
+ "Sent": q.Get("sent") == "1",
+ "Error": signinError(q.Get("err")),
+ // rastrillo sends a failed keymail round trip back with
+ // force=1&err=keymail so the page can offer plain email instead;
+ // the hidden field makes the next submission skip keymail.
+ "ForceEmail": q.Get("force") == "1",
})
}
@@ -286,3 +291,28 @@ func (a *app) fail(w http.ResponseWriter, r *http.Request, err error) {
a.logger.Error("request failed", "path", r.URL.Path, "err", err)
ErrorPage(w, r, http.StatusInternalServerError, "")
}
+
+// signinError turns rastrillo's ?err= codes into something a person can
+// act on. The first version showed "That did not work — try again in a
+// moment" for every code, including a missing table, which is advice that
+// can never succeed. AGENTS.md: errors in plain language.
+//
+// None of these may distinguish an admitted address from a stranger's —
+// beginGuard exists to make those look identical — so "address" means
+// malformed, never "not allowed".
+func signinError(code string) string {
+ switch code {
+ case "":
+ return ""
+ case "rate":
+ return "Too many sign-in attempts for that address. Wait a few minutes, then try again."
+ case "address":
+ return "That doesn't look like an email address. Check it and try again."
+ case "expired":
+ return "That sign-in link has expired or has already been used. Ask for a new one below."
+ case "keymail":
+ return "Keymail couldn't finish signing you in. You can have a link emailed to you instead."
+ default:
+ return "Something went wrong on our side, not yours. Trying again won't help until it is fixed."
+ }
+}
diff --git a/internal/fichas/migrations.go b/internal/fichas/migrations.go
index 1e4e3ec..b3cf739 100644
--- a/internal/fichas/migrations.go
+++ b/internal/fichas/migrations.go
@@ -3,7 +3,9 @@ package fichas
import (
"embed"
+ "github.com/carlosframework/rastrillo/auth"
"github.com/carlosframework/rastrillo/migrate"
+ "github.com/carlosframework/rastrillo/sessions"
)
//go:embed migrations/*.sql
@@ -39,4 +41,14 @@ var Schema = migrate.MustFromFS(migrationFS, "fichas")
// migrations another one reads (auth after sessions) goes first.
// Get this line wrong and either boot silently skips a subsystem's
// tables, or generate/check try to drop them.
-var BootSchema = migrate.Merge(Schema)
+// sessions and auth come first: auth's tables reference nothing of ours,
+// and ours reference nothing of theirs, but apply order is argument order
+// and theirs are the ones every request depends on.
+//
+// **This line was `migrate.Merge(Schema)` for the first eight days in
+// production.** An earlier edit meant to add the two subsystems matched a
+// different spelling of the line, changed nothing, and was never checked —
+// so auth_links and the sessions table did not exist, and no sign-in on
+// the deployed app could complete by either path. TestBootSchema and
+// TestASignInCompletesEndToEnd are what stop that happening silently again.
+var BootSchema = migrate.Merge(sessions.Schema, auth.Schema, Schema)
diff --git a/internal/fichas/templates/signin.html b/internal/fichas/templates/signin.html
index 70946db..56039a1 100644
--- a/internal/fichas/templates/signin.html
+++ b/internal/fichas/templates/signin.html
@@ -14,8 +14,9 @@
"Title" "Check your email"
"Body" "If that address can sign in here, a link is on its way. The link works once and expires.")}}
{{else}}
- {{if .Error}}{{template "callout" (dict "Tone" "negative" "Title" "That did not work" "Body" "Try again in a moment.")}}{{end}}
+ {{with .Error}}{{template "callout" (dict "Tone" "negative" "Title" "That didn't work" "Body" .)}}{{end}}
<form method="post" action="/auth/begin">
+ {{if .ForceEmail}}<input type="hidden" name="force" value="1">{{end}}
{{template "field" (dict
"ID" "address" "Name" "address" "Label" "Your email address"
"Type" "email" "Autocomplete" "email" "Required" true "Autofocus" true
diff --git a/internal/fichastest/signin_flow_test.go b/internal/fichastest/signin_flow_test.go
new file mode 100644
index 0000000..dbc9fee
--- /dev/null
+++ b/internal/fichastest/signin_flow_test.go
@@ -0,0 +1,193 @@
+package fichastest
+
+import (
+ "context"
+ "io"
+ "log/slog"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "path/filepath"
+ "regexp"
+ "strings"
+ "sync"
+ "testing"
+
+ fichas "amadan.net/oficina/fichas/internal/fichas"
+ "github.com/carlosframework/rastrillo/db"
+)
+
+// capturingMailer records what the app sends instead of sending it.
+type capturingMailer struct {
+ mu sync.Mutex
+ sent []string
+}
+
+func (m *capturingMailer) Send(_ context.Context, to, subject, body string) error {
+ m.mu.Lock()
+ defer m.mu.Unlock()
+ m.sent = append(m.sent, to+"\n"+subject+"\n"+body)
+ return nil
+}
+
+func (m *capturingMailer) last(t *testing.T) string {
+ t.Helper()
+ m.mu.Lock()
+ defer m.mu.Unlock()
+ if len(m.sent) == 0 {
+ t.Fatal("no email was sent")
+ }
+ return m.sent[len(m.sent)-1]
+}
+
+// wiredApp is the app exactly as main.go builds it — App, then NewAuth,
+// then UseAuth — with a capturing mailer in place of SMTP and NO test
+// identity or bypass. Everything that reached production went through
+// this sequence, and until this test nothing in the suite did.
+func wiredApp(t *testing.T) (http.Handler, *capturingMailer, *db.DB) {
+ t.Helper()
+ logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+ d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger)
+ if err != nil {
+ t.Fatalf("db.Open: %v", err)
+ }
+ t.Cleanup(func() { d.Close() })
+
+ cfg := testConfig(t)
+ mux, app, err := fichas.App(d, cfg, logger)
+ if err != nil {
+ t.Fatalf("App: %v", err)
+ }
+ mailer := &capturingMailer{}
+ au, err := fichas.NewAuth(d.Writer(), fichas.AuthEnv{
+ Origin: cfg.Origin,
+ InstanceKey: cfg.InstanceKey,
+ Mailer: mailer,
+ }, logger, app.Admit)
+ if err != nil {
+ t.Fatalf("NewAuth: %v", err)
+ }
+ app.UseAuth(au)
+ return mux, mailer, d
+}
+
+// TestBootSchema: the tables sign-in depends on exist after a boot.
+//
+// BootSchema was migrate.Merge(Schema) for eight days in production: an
+// edit meant to add sessions and auth matched a different spelling of the
+// line and changed nothing. Every boot log said skipped=2 and nobody read
+// it as "two, not six".
+func TestBootSchema(t *testing.T) {
+ _, _, d := wiredApp(t)
+ for _, table := range []string{"auth_links", "sessions", "cards", "events"} {
+ var n int64
+ if err := d.G.Raw("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = ?", table).Scan(&n).Error; err != nil {
+ t.Fatalf("sqlite_master: %v", err)
+ }
+ if n != 1 {
+ t.Errorf("table %s does not exist after boot", table)
+ }
+ }
+}
+
+var linkToken = regexp.MustCompile(`/auth/verify\?token=([A-Za-z0-9_\-%.~]+)`)
+
+// TestASignInCompletesEndToEnd walks the email path the way a person does:
+// submit an address, receive a link, follow it, arrive signed in.
+//
+// No earlier test did this. They either bypassed auth or stopped at the
+// guard in front of it, so nothing exercised the path that touches the
+// database — and the deployed app could not complete a single sign-in.
+func TestASignInCompletesEndToEnd(t *testing.T) {
+ h, mailer, _ := wiredApp(t)
+
+ // force=1 takes the plain-email path, which skips the classifier and
+ // so makes no network request: this test must not dial anything.
+ rec := post(t, h, "/auth/begin", url.Values{"address": {TestOwner}, "force": {"1"}})
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST /auth/begin: status %d\n%s", rec.Code, rec.Body.String())
+ }
+ if loc := rec.Header().Get("Location"); loc != "/signin?sent=1" {
+ t.Fatalf("POST /auth/begin redirected to %q, want /signin?sent=1", loc)
+ }
+
+ mail := mailer.last(t)
+ if !strings.HasPrefix(mail, TestOwner+"\n") {
+ t.Fatalf("the link went to the wrong address:\n%s", mail)
+ }
+ m := linkToken.FindStringSubmatch(mail)
+ if m == nil {
+ t.Fatalf("the email carries no sign-in link:\n%s", mail)
+ }
+ // The link must point at the configured origin, or it goes nowhere.
+ if !strings.Contains(mail, testOrigin+"/auth/verify?token=") {
+ t.Errorf("the link does not use FICHAS_ORIGIN:\n%s", mail)
+ }
+
+ verify := get(t, h, "/auth/verify?token="+m[1])
+ if verify.Code != http.StatusSeeOther {
+ t.Fatalf("GET /auth/verify: status %d\n%s", verify.Code, verify.Body.String())
+ }
+ if loc := verify.Header().Get("Location"); strings.Contains(loc, "err=") {
+ t.Fatalf("verify failed: redirected to %q", loc)
+ }
+ cookies := verify.Result().Cookies()
+ if len(cookies) == 0 {
+ t.Fatal("verify set no session cookie")
+ }
+
+ req := httptest.NewRequest(http.MethodGet, "/", nil)
+ for _, c := range cookies {
+ req.AddCookie(c)
+ }
+ home := httptest.NewRecorder()
+ h.ServeHTTP(home, req)
+ if home.Code != http.StatusOK {
+ t.Fatalf("GET / with the new session: status %d, want 200 (Location %q)",
+ home.Code, home.Header().Get("Location"))
+ }
+ if !strings.Contains(home.Body.String(), "Customers") {
+ t.Error("signed in, but the index did not render")
+ }
+
+ // A link is single-use.
+ again := get(t, h, "/auth/verify?token="+m[1])
+ if loc := again.Header().Get("Location"); !strings.Contains(loc, "err=expired") {
+ t.Errorf("a used link was accepted a second time (redirected to %q)", loc)
+ }
+}
+
+// A stranger must get the same outcome and no email — the anti-oracle
+// property — and the wiring must not change that.
+func TestAStrangerIsSentNothing(t *testing.T) {
+ h, mailer, _ := wiredApp(t)
+ rec := post(t, h, "/auth/begin", url.Values{"address": {"nobody@example.com"}, "force": {"1"}})
+ if loc := rec.Header().Get("Location"); loc != "/signin?sent=1" {
+ t.Fatalf("a stranger was redirected to %q", loc)
+ }
+ mailer.mu.Lock()
+ defer mailer.mu.Unlock()
+ if len(mailer.sent) != 0 {
+ t.Fatalf("a stranger was sent %d email(s)", len(mailer.sent))
+ }
+}
+
+// Every error code renders something a person can act on, and the email
+// fallback survives a failed keymail round trip.
+func TestSigninErrorsArePlainLanguage(t *testing.T) {
+ h, _, _ := wiredApp(t)
+ for code, want := range map[string]string{
+ "rate": "Too many sign-in attempts",
+ "expired": "has expired or has already been used",
+ "keymail": "Keymail couldn",
+ "1": "Something went wrong on our side",
+ } {
+ body := get(t, h, "/signin?err="+code).Body.String()
+ if !strings.Contains(body, want) {
+ t.Errorf("?err=%s does not say %q", code, want)
+ }
+ }
+ if !strings.Contains(get(t, h, "/signin?force=1&err=keymail").Body.String(), `name="force" value="1"`) {
+ t.Error("after a keymail failure the form does not fall back to email")
+ }
+}