amadan

oficina / sheets Public

feat: magic-link sign-in

Mounts rastrillo/auth, matching what Docs built rather than inventing a
second flow. Tokens are hashed, single-use (TakeLink's DELETE ...
RETURNING makes the delete the check, so two simultaneous redemptions
cannot both win), and expire with a sweeper. keymail is handled by the
plugin, so it works without extra wiring here.

Sign-in is instance-local and the suite home is not involved. That is
not a concession to slice one: a passkey binds a credential to a relying
party, which is what forced a single RP at the home, and a magic link
binds nothing. Requiring someone self-hosting one instance to stand up
the whole suite home in order to log in would make factor VII nominal.

Boot refuses rather than degrades, because each of these produces an
instance that serves pages and answers /healthz while being unsafe:
no SHEETS_ORIGIN, no SHEETS_INSTANCE_KEY, an empty SHEETS_OWNERS
allowlist (a nil Authorize admits everyone who can receive mail), or no
SMTP (mail.FromEnv degrades to logging the link, and the link is a live
credential going to platform-held storage we cannot grep or redact).

The one relaxation is bound to a fact rather than a switch: the logging
mailer is reachable only when the origin is a loopback host, which a
deployed instance's public https origin cannot be. There is no
development flag or variable, so there is nothing to set by accident in
production, and an instance that lies about it mints links pointing at
localhost — loud, not quiet.

That check was written with strings.HasPrefix and was wrong:
"http://127.0.0.1.attacker.example" is a domain someone else can own and
it passed. TestLoopbackIsNotPrefixSpoofable caught it before it was
committed; the host is now compared after parsing.

The sign-in page has no generic fall-through. auth's Verify consumes the
row, so clicking one expired link twice gives two different outcomes —
the second finds no row and reports a generic failure, which is the
commonest real case (an old link in an earlier email) and exactly when
dropping the "ask for a new one" instruction strands someone. Docs found
this on their page; it comes from auth's redirect vocabulary, so any app
mounting these handlers inherits it. No query value is interpolated into
the page at all: the message is selected from a fixed set, never echoed.

SHEETS_OWNERS is a deliberate stand-in for a membership table — the
smallest thing that is fail-closed, not the shape sharing will need.

Migration order is load-bearing: auth's backfill reads the sessions
table, so BootSchema merges sessions.Schema before auth.Schema.

TestAuthRoutesAreMounted was verified by unmounting the routes and
watching it fail on all four, with TestAuthRoutesAbsentWithoutAuth as
the control that 404/405 really does mean "not mounted".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
Paul Campbell pushed by paul@keymail.dev 52c517517db18e8a5bd32d3d7061072d35d6d5d4 parent 779e481
10 files changed, +584 −12
  • cmd/sheets/main.go +19 −1
  • go.mod +1 −0
  • internal/sheets/app.go +21 −2
  • internal/sheets/app_test.go +1 −1
  • internal/sheets/auth.go +166 −0
  • internal/sheets/auth_test.go +161 −0
  • internal/sheets/migrations.go +9 −7
  • internal/sheets/page_test.go +1 −1
  • internal/sheets/signin.go +84 −0
  • internal/sheets/signin_test.go +121 −0

Intent

full ledger: core

diff --git a/cmd/sheets/main.go b/cmd/sheets/main.go
index 0637c5d..61c598e 100644
--- a/cmd/sheets/main.go
+++ b/cmd/sheets/main.go
@@ -62,7 +62,25 @@ func main() {
os.Exit(1)
}
- app := sheets.New(d.G)
+ // Sign-in configuration is validated before anything serves. Every
+ // failure here is one where continuing would produce an instance
+ // that looks like it works: no mailer means magic links get written
+ // to platform-held logs instead of emailed, and an empty allowlist
+ // means anyone who can receive mail can hold a session. Both answer
+ // /healthz perfectly well, which is why this is a hard exit and not
+ // a warning.
+ authEnv, err := sheets.LoadAuthEnv(logger)
+ if err != nil {
+ logger.Error("sign-in", "err", err)
+ os.Exit(1)
+ }
+ signin, err := sheets.NewAuth(d.Writer(), authEnv, logger)
+ if err != nil {
+ logger.Error("sign-in", "err", err)
+ os.Exit(1)
+ }
+
+ app := sheets.New(d.G, signin)
// rastrillo.Options.Mux wants a *http.ServeMux; the app's own
// router is a chi.Router (an http.Handler), so it mounts under one.
diff --git a/go.mod b/go.mod
index 97350b2..1c8fc5a 100644
--- a/go.mod
+++ b/go.mod
@@ -15,6 +15,7 @@ require (
github.com/google/uuid v1.6.0 // indirect
github.com/jinzhu/inflection v1.0.0 // indirect
github.com/jinzhu/now v1.1.5 // indirect
+ github.com/keymaildev/signin v0.1.1 // indirect
github.com/mattn/go-isatty v0.0.20 // indirect
github.com/ncruces/go-strftime v1.0.0 // indirect
github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
diff --git a/internal/sheets/app.go b/internal/sheets/app.go
index 4520f71..78597f7 100644
--- a/internal/sheets/app.go
+++ b/internal/sheets/app.go
@@ -5,17 +5,25 @@ package sheets
import (
"net/http"
+ "github.com/carlosframework/rastrillo/auth"
"github.com/go-chi/chi/v5"
"gorm.io/gorm"
)
// App holds the wiring. One per process.
type App struct {
- db *gorm.DB
+ db *gorm.DB
+ auth *auth.Auth
}
// New builds an App. db may be nil in tests that do not touch storage.
-func New(db *gorm.DB) *App { return &App{db: db} }
+//
+// a may be nil ONLY in tests: Router then serves the sign-in page but
+// mounts none of auth's handlers, so a nil-auth App can render pages
+// without a database or a mailer. Production always passes one — main
+// refuses to boot without the configuration auth needs, so there is no
+// path there that reaches this with nil.
+func New(db *gorm.DB, a *auth.Auth) *App { return &App{db: db, auth: a} }
// Router returns the mounted handler.
func (a *App) Router() http.Handler {
@@ -34,5 +42,16 @@ func (a *App) Router() http.Handler {
// other path, which is what an operator hitting a wrong URL
// should see (TestIndexIsNotACatchAll pins this).
r.Get("/", a.handleIndex)
+ r.Get("/signin", a.handleSignin)
+ if a.auth != nil {
+ // The four handlers rastrillo/auth expects an app to mount, at
+ // the paths its own docs use. Verify reads ?token=; the path is
+ // baked into every emailed link, so moving it later invalidates
+ // links already in people's inboxes.
+ r.Post("/signin", a.auth.Begin)
+ r.Get("/auth/callback", a.auth.Callback)
+ r.Get("/auth/verify", a.auth.Verify)
+ r.Post("/signout", a.auth.Signout)
+ }
return r
}
diff --git a/internal/sheets/app_test.go b/internal/sheets/app_test.go
index b327c56..d6007c0 100644
--- a/internal/sheets/app_test.go
+++ b/internal/sheets/app_test.go
@@ -12,7 +12,7 @@ import (
// ahead of this one and wins (see the comment on Router in app.go); this
// test only proves the app's router itself is wired and reachable.
func TestHealthz(t *testing.T) {
- app := New(nil)
+ app := New(nil, nil)
req := httptest.NewRequest(http.MethodGet, "/healthz", nil)
rec := httptest.NewRecorder()
app.Router().ServeHTTP(rec, req)
diff --git a/internal/sheets/auth.go b/internal/sheets/auth.go
new file mode 100644
index 0000000..011afc6
--- /dev/null
+++ b/internal/sheets/auth.go
@@ -0,0 +1,166 @@
+package sheets
+
+import (
+ "database/sql"
+ "errors"
+ "fmt"
+ "log/slog"
+ "net/url"
+ "os"
+ "strings"
+
+ "github.com/carlosframework/rastrillo/auth"
+ "github.com/carlosframework/rastrillo/mail"
+)
+
+// AuthEnv is everything sign-in needs from the environment, already
+// validated. Building it is the only place these variable names appear.
+type AuthEnv struct {
+ // Origin is the instance's external origin, scheme included. It is
+ // the base of every emailed link and the CSRF origin check, which
+ // makes a WRONG value the dangerous case rather than a missing one:
+ // a missing origin refuses at boot, while a wrong one boots happily
+ // and mints links pointing at a host nobody serves. Set it only
+ // after the route exists and answers.
+ Origin string
+
+ // InstanceKey seals auth's pending blob. auth.New refuses an empty
+ // one, but it refuses at the wiring call with a framework message;
+ // this refuses at boot naming the variable an operator has to set.
+ InstanceKey string
+
+ // Owners is the admission allowlist: the addresses that may hold a
+ // session on this instance.
+ Owners []string
+
+ // Mailer delivers the links.
+ Mailer mail.Sender
+}
+
+// loopback reports whether origin is a local development origin.
+//
+// This is the ONLY thing that relaxes the mail requirement, and it is
+// deliberately a fact rather than a switch. A deployed instance's origin
+// is its public https host, so it cannot take this branch; there is no
+// flag or variable that turns "development" on, so there is nothing to
+// set by accident in production. And the failure mode of lying about it
+// is loud rather than quiet — an instance claiming a loopback origin
+// mints links pointing at localhost, which nobody can click.
+// The host is compared after parsing, never by prefix. A prefix test on
+// "http://127.0.0.1" also accepts "http://127.0.0.1.attacker.example",
+// which is a real domain someone else can own — caught here by
+// TestLoopbackIsNotPrefixSpoofable, which failed on exactly that string
+// when this was written with strings.HasPrefix.
+func loopback(origin string) bool {
+ u, err := url.Parse(origin)
+ if err != nil || u.Scheme != "http" {
+ return false
+ }
+ switch u.Hostname() {
+ case "localhost", "127.0.0.1", "::1":
+ return true
+ }
+ return false
+}
+
+// LoadAuthEnv reads and validates sign-in configuration, refusing rather
+// than degrading.
+//
+// Every refusal here is a case where continuing would produce an app
+// that looks like it works. An instance with no mailer logs live
+// credentials to platform-held storage instead of emailing them; an
+// instance with an empty allowlist admits anyone who can receive mail at
+// any address. Both serve pages and answer /healthz.
+func LoadAuthEnv(logger *slog.Logger) (AuthEnv, error) {
+ env := AuthEnv{
+ Origin: strings.TrimSpace(os.Getenv("SHEETS_ORIGIN")),
+ InstanceKey: strings.TrimSpace(os.Getenv("SHEETS_INSTANCE_KEY")),
+ Owners: splitOwners(os.Getenv("SHEETS_OWNERS")),
+ }
+
+ var missing []string
+ if env.Origin == "" {
+ missing = append(missing, "SHEETS_ORIGIN")
+ }
+ if env.InstanceKey == "" {
+ missing = append(missing, "SHEETS_INSTANCE_KEY")
+ }
+ if len(env.Owners) == 0 {
+ missing = append(missing, "SHEETS_OWNERS")
+ }
+ if len(missing) > 0 {
+ return AuthEnv{}, fmt.Errorf("sign-in is not configured: %s not set",
+ strings.Join(missing, ", "))
+ }
+
+ // mail.FromEnv falls back to a logging sender with a warning when
+ // SMTP is absent. A warning is not enough: the emailed link is a
+ // live credential and the log is platform-held storage we cannot
+ // grep or redact. Detect the fallback ourselves and refuse.
+ if os.Getenv("CARLOS_SMTP_HOST") == "" || os.Getenv("CARLOS_SMTP_FROM") == "" {
+ if !loopback(env.Origin) {
+ return AuthEnv{}, errors.New(
+ "sign-in is not configured: CARLOS_SMTP_HOST and CARLOS_SMTP_FROM not set, " +
+ "so magic links would be written to the log instead of emailed. " +
+ "Run `carlos email enable --app sheets`")
+ }
+ logger.Warn("sign-in: no SMTP configured and origin is loopback — " +
+ "magic links will be LOGGED, NOT SENT")
+ }
+ env.Mailer = mail.FromEnv("CARLOS", logger)
+
+ return env, nil
+}
+
+// splitOwners parses the allowlist, lowercasing and dropping blanks so a
+// trailing comma or stray space cannot admit or exclude anyone by
+// accident.
+func splitOwners(raw string) []string {
+ var out []string
+ for _, part := range strings.Split(raw, ",") {
+ if addr := strings.ToLower(strings.TrimSpace(part)); addr != "" {
+ out = append(out, addr)
+ }
+ }
+ return out
+}
+
+// NewAuth wires the sign-in flow. Exactly one per process: auth's
+// underlying flow carries rate-limiter state on the value, so one per
+// request would get a fresh empty budget every time and rate-limit
+// nothing, silently.
+func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger) (*auth.Auth, error) {
+ return auth.New(auth.Config{
+ DB: db,
+ Origin: env.Origin,
+ InstanceKey: env.InstanceKey,
+ Mailer: env.Mailer,
+ Authorize: admits(env.Owners),
+ SigninPath: "/signin",
+ Subject: "Your Sheets sign-in link",
+ Logger: logger,
+ })
+}
+
+// admits builds the admission gate.
+//
+// A nil Authorize admits every verified address, which on a
+// public-hostname instance means anyone who can receive mail. The
+// allowlist is a slice-one stand-in for a membership table: it is
+// deliberately the smallest thing that is fail-closed, not the shape
+// sharing will eventually need.
+//
+// Comparison is on the lowercased address. That is not full
+// normalisation — addresses are case-sensitive before the @ per RFC
+// 5321, and no mainstream provider honours that — so this matches what
+// operators expect when they type an allowlist.
+func admits(owners []string) func(string) bool {
+ allowed := make(map[string]struct{}, len(owners))
+ for _, o := range owners {
+ allowed[o] = struct{}{}
+ }
+ return func(address string) bool {
+ _, ok := allowed[strings.ToLower(strings.TrimSpace(address))]
+ return ok
+ }
+}
diff --git a/internal/sheets/auth_test.go b/internal/sheets/auth_test.go
new file mode 100644
index 0000000..1680408
--- /dev/null
+++ b/internal/sheets/auth_test.go
@@ -0,0 +1,161 @@
+package sheets
+
+import (
+ "io"
+ "log/slog"
+ "strings"
+ "testing"
+)
+
+func quietLogger() *slog.Logger {
+ return slog.New(slog.NewTextHandler(io.Discard, nil))
+}
+
+// setEnv clears every variable LoadAuthEnv reads, then applies the given
+// ones. Clearing first matters: the process running these tests may have
+// real values in its environment, and a test that passed only because a
+// developer had SHEETS_ORIGIN exported would be worthless.
+func setEnv(t *testing.T, kv map[string]string) {
+ t.Helper()
+ for _, k := range []string{
+ "SHEETS_ORIGIN", "SHEETS_INSTANCE_KEY", "SHEETS_OWNERS",
+ "CARLOS_SMTP_HOST", "CARLOS_SMTP_FROM",
+ } {
+ t.Setenv(k, "")
+ }
+ for k, v := range kv {
+ t.Setenv(k, v)
+ }
+}
+
+func goodEnv() map[string]string {
+ return map[string]string{
+ "SHEETS_ORIGIN": "https://sheets.example.com",
+ "SHEETS_INSTANCE_KEY": "not-empty",
+ "SHEETS_OWNERS": "paul@example.com",
+ "CARLOS_SMTP_HOST": "smtp.example.com",
+ "CARLOS_SMTP_FROM": "noreply@sheets.example.com",
+ }
+}
+
+func TestLoadAuthEnvAcceptsCompleteConfig(t *testing.T) {
+ setEnv(t, goodEnv())
+ env, err := LoadAuthEnv(quietLogger())
+ if err != nil {
+ t.Fatalf("LoadAuthEnv: %v", err)
+ }
+ if env.Origin != "https://sheets.example.com" {
+ t.Errorf("Origin = %q", env.Origin)
+ }
+ if env.Mailer == nil {
+ t.Error("Mailer is nil")
+ }
+}
+
+// TestLoadAuthEnvRefusesEachMissingVar is the boot gate. Each of these
+// would otherwise produce an instance that serves pages and answers
+// /healthz while being unable to sign anyone in safely.
+func TestLoadAuthEnvRefusesEachMissingVar(t *testing.T) {
+ for _, missing := range []string{"SHEETS_ORIGIN", "SHEETS_INSTANCE_KEY", "SHEETS_OWNERS"} {
+ t.Run(missing, func(t *testing.T) {
+ env := goodEnv()
+ delete(env, missing)
+ setEnv(t, env)
+ _, err := LoadAuthEnv(quietLogger())
+ if err == nil {
+ t.Fatalf("missing %s: accepted, want refusal", missing)
+ }
+ if !strings.Contains(err.Error(), missing) {
+ t.Errorf("error does not name %s: %v", missing, err)
+ }
+ })
+ }
+}
+
+// TestLoadAuthEnvRefusesMissingMail: mail.FromEnv degrades to a logging
+// sender with a warning. The link is a live credential and the log is
+// platform-held storage, so degrading is worse than refusing.
+func TestLoadAuthEnvRefusesMissingMail(t *testing.T) {
+ for _, missing := range []string{"CARLOS_SMTP_HOST", "CARLOS_SMTP_FROM"} {
+ t.Run(missing, func(t *testing.T) {
+ env := goodEnv()
+ delete(env, missing)
+ setEnv(t, env)
+ if _, err := LoadAuthEnv(quietLogger()); err == nil {
+ t.Fatalf("missing %s: accepted, want refusal", missing)
+ }
+ })
+ }
+}
+
+// TestLoadAuthEnvAllowsLoggedMailOnLoopback pins the one relaxation, and
+// pins that it is bound to the origin rather than to a switch. A
+// deployed instance's origin is its public https host, so it cannot
+// reach this branch.
+func TestLoadAuthEnvAllowsLoggedMailOnLoopback(t *testing.T) {
+ for _, origin := range []string{"http://localhost:8080", "http://127.0.0.1:8080"} {
+ t.Run(origin, func(t *testing.T) {
+ env := goodEnv()
+ env["SHEETS_ORIGIN"] = origin
+ delete(env, "CARLOS_SMTP_HOST")
+ delete(env, "CARLOS_SMTP_FROM")
+ setEnv(t, env)
+ if _, err := LoadAuthEnv(quietLogger()); err != nil {
+ t.Fatalf("loopback origin with no SMTP: %v, want accepted", err)
+ }
+ })
+ }
+}
+
+// TestLoopbackIsNotPrefixSpoofable: the relaxation must not be reachable
+// by a hostname that merely starts with the loopback text.
+func TestLoopbackIsNotPrefixSpoofable(t *testing.T) {
+ for _, origin := range []string{
+ "https://localhost.attacker.example",
+ "http://127.0.0.1.attacker.example",
+ "http://localhost.attacker.example",
+ "http://127.0.0.1.example.com",
+ "http://notlocalhost",
+ } {
+ t.Run(origin, func(t *testing.T) {
+ env := goodEnv()
+ env["SHEETS_ORIGIN"] = origin
+ delete(env, "CARLOS_SMTP_HOST")
+ delete(env, "CARLOS_SMTP_FROM")
+ setEnv(t, env)
+ if _, err := LoadAuthEnv(quietLogger()); err == nil {
+ t.Errorf("%s reached the loopback relaxation", origin)
+ }
+ })
+ }
+}
+
+func TestAdmitsIsFailClosed(t *testing.T) {
+ allow := admits(splitOwners("Paul@Example.com, other@example.com,"))
+ for _, tc := range []struct {
+ address string
+ want bool
+ }{
+ {"paul@example.com", true},
+ {"PAUL@EXAMPLE.COM", true},
+ {" paul@example.com ", true},
+ {"other@example.com", true},
+ {"stranger@example.com", false},
+ {"", false},
+ } {
+ if got := allow(tc.address); got != tc.want {
+ t.Errorf("admits(%q) = %v, want %v", tc.address, got, tc.want)
+ }
+ }
+}
+
+// TestSplitOwnersDropsBlanks: a trailing comma or a stray space must not
+// put an empty string in the allowlist, which admits("") would then let
+// through.
+func TestSplitOwnersDropsBlanks(t *testing.T) {
+ for _, raw := range []string{"", " ", ",", " , , "} {
+ if got := splitOwners(raw); len(got) != 0 {
+ t.Errorf("splitOwners(%q) = %v, want empty", raw, got)
+ }
+ }
+}
diff --git a/internal/sheets/migrations.go b/internal/sheets/migrations.go
index 7bfb392..23af022 100644
--- a/internal/sheets/migrations.go
+++ b/internal/sheets/migrations.go
@@ -3,7 +3,9 @@ package sheets
import (
"embed"
+ "github.com/carlosframework/rastrillo/auth"
"github.com/carlosframework/rastrillo/migrate"
+ "github.com/carlosframework/rastrillo/sessions"
)
// migrationsFS embeds internal/sheets/migrations/*.sql — this package's
@@ -26,13 +28,13 @@ var Schema = migrate.MustFromFS(migrationsFS, "sheets")
// BootSchema is everything migrate.Apply runs at boot, in apply order —
// the shape SKILL.md documents as `migrate.Merge(subsystemSchemas...,
-// Schema)`. There is no framework subsystem to merge in here (this app
-// uses none of rastrillo's sessions/auth/jobs pieces), so BootSchema is
-// Schema alone; migrate.Merge is still the call, not a bare assignment,
-// so a future subsystem's schema has one obvious place to be added
-// without anyone needing to notice BootSchema didn't previously go
-// through Merge at all.
-var BootSchema = migrate.Merge(Schema)
+// Schema)`.
+//
+// The order is not a preference. auth's backfill migration reads the
+// sessions table, so sessions.Schema must apply first; migrate.Merge's
+// argument order IS apply order. Sheets' own Schema comes last because
+// it depends on neither.
+var BootSchema = migrate.Merge(sessions.Schema, auth.Schema, Schema)
// Models lists every GORM model `rastrillo migration generate`/`check`
// diff Schema against — Sheet, Tab, Row, Col, Cell and Format from
diff --git a/internal/sheets/page_test.go b/internal/sheets/page_test.go
index e1c9ea6..5771117 100644
--- a/internal/sheets/page_test.go
+++ b/internal/sheets/page_test.go
@@ -10,7 +10,7 @@ import (
func get(t *testing.T, path string) *httptest.ResponseRecorder {
t.Helper()
rec := httptest.NewRecorder()
- New(nil).Router().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil))
+ New(nil, nil).Router().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil))
return rec
}
diff --git a/internal/sheets/signin.go b/internal/sheets/signin.go
new file mode 100644
index 0000000..cb005ed
--- /dev/null
+++ b/internal/sheets/signin.go
@@ -0,0 +1,84 @@
+package sheets
+
+import "net/http"
+
+// signinMessage maps auth's redirect vocabulary to what the person in
+// front of the screen needs to do next.
+//
+// The default case is the point of this function. On the sign-in page
+// every outcome means one operational thing — we could not sign you in,
+// ask for a new link — so an unrecognised code must say that too, not
+// fall through to an app-wide "something went wrong".
+//
+// That is not hypothetical tidiness. auth's Verify consumes the link row
+// with DELETE ... RETURNING, so clicking one expired link twice produces
+// two DIFFERENT outcomes: the first finds an expired row (?err=expired),
+// the second finds no row at all and reports a generic failure. The
+// second is the more common real case — someone clicking an old link out
+// of an earlier email — and it is exactly when dropping the "request a
+// new one" instruction leaves them with nowhere to go. Docs found this
+// on their own sign-in page; it comes from the shape of auth's
+// vocabulary, so any app mounting these handlers inherits it.
+func signinMessage(q map[string][]string) string {
+ first := func(k string) string {
+ if v := q[k]; len(v) > 0 {
+ return v[0]
+ }
+ return ""
+ }
+ if first("sent") != "" {
+ return "Check your email. The link signs you in and works once."
+ }
+ switch first("err") {
+ case "":
+ return ""
+ case "rate":
+ return "Too many sign-in attempts. Wait a few minutes, then ask for a new link."
+ case "address":
+ return "That address cannot sign in to this instance. Ask whoever runs it to add you."
+ case "expired":
+ return "That link has expired or was already used. Ask for a new one below."
+ case "keymail":
+ return "Your keymail sign-in could not be completed. Ask for an emailed link instead."
+ default:
+ return "We could not sign you in. Ask for a new link below."
+ }
+}
+
+// signinPage is assembled from constants only — no query value is ever
+// interpolated into it. signinMessage SELECTS one of a fixed set of
+// strings rather than echoing what it was given, so there is no path
+// from the URL into the HTML and nothing here to escape.
+const (
+ signinHead = `<!doctype html>
+<html lang="en">
+<head>
+<meta charset="utf-8">
+<title>Sign in — Oficina Sheets</title>
+</head>
+<body>
+<main>
+<h1>Sign in to Oficina Sheets</h1>
+`
+ signinForm = `<form method="post" action="/signin">
+<label for="address">Email address</label>
+<input id="address" name="address" type="email" autocomplete="email" required>
+<button type="submit">Email me a link</button>
+</form>
+<p>We email you a link instead of asking for a password. It signs you in
+once and then stops working.</p>
+</main>
+</body>
+</html>
+`
+)
+
+// handleSignin serves the sign-in page.
+func (a *App) handleSignin(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "text/html; charset=utf-8")
+ w.Write([]byte(signinHead))
+ if msg := signinMessage(r.URL.Query()); msg != "" {
+ w.Write([]byte("<p>" + msg + "</p>\n"))
+ }
+ w.Write([]byte(signinForm))
+}
diff --git a/internal/sheets/signin_test.go b/internal/sheets/signin_test.go
new file mode 100644
index 0000000..0a68db0
--- /dev/null
+++ b/internal/sheets/signin_test.go
@@ -0,0 +1,121 @@
+package sheets
+
+import (
+ "context"
+ "net/http"
+ "net/http/httptest"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ rdb "github.com/carlosframework/rastrillo/db"
+ "github.com/carlosframework/rastrillo/migrate"
+)
+
+// TestSigninMessageNeverFallsThroughSilently is the reason this function
+// exists. Every outcome on this page means one operational thing, so an
+// unrecognised code must still tell the person what to do — the second
+// click on an expired link produces exactly such a code, and it is the
+// commonest real failure.
+func TestSigninMessageNeverFallsThroughSilently(t *testing.T) {
+ for _, code := range []string{"1", "keymail", "expired", "rate", "address", "wat", "0"} {
+ msg := signinMessage(map[string][]string{"err": {code}})
+ if msg == "" {
+ t.Errorf("err=%s produced no message", code)
+ continue
+ }
+ // Every error path has to leave a way forward.
+ low := strings.ToLower(msg)
+ if !strings.Contains(low, "new link") && !strings.Contains(low, "new one") &&
+ !strings.Contains(low, "emailed link") && !strings.Contains(low, "add you") {
+ t.Errorf("err=%s says %q, with no next step", code, msg)
+ }
+ }
+}
+
+func TestSigninMessageSentAndEmpty(t *testing.T) {
+ if msg := signinMessage(map[string][]string{"sent": {"1"}}); !strings.Contains(msg, "Check your email") {
+ t.Errorf("sent=1 gave %q", msg)
+ }
+ if msg := signinMessage(map[string][]string{}); msg != "" {
+ t.Errorf("bare page gave %q, want no message", msg)
+ }
+}
+
+// TestSigninPageDoesNotEchoQuery: signinMessage selects from a fixed set
+// rather than echoing, so there is no path from the URL into the HTML.
+func TestSigninPageDoesNotEchoQuery(t *testing.T) {
+ rec := httptest.NewRecorder()
+ req := httptest.NewRequest(http.MethodGet, "/signin?err=%3Cscript%3Ealert(1)%3C/script%3E", nil)
+ New(nil, nil).Router().ServeHTTP(rec, req)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("GET /signin = %d", rec.Code)
+ }
+ if strings.Contains(rec.Body.String(), "<script>") {
+ t.Error("query value reached the page")
+ }
+ if !strings.Contains(rec.Body.String(), "could not sign you in") {
+ t.Error("unknown err code did not get the fallback message")
+ }
+}
+
+// testAuth builds a real *auth.Auth over a temp database, so route
+// mounting is proven against the actual handlers rather than a stub.
+func testAuth(t *testing.T) *App {
+ t.Helper()
+ d, err := rdb.Open(filepath.Join(t.TempDir(), "test.db"), quietLogger())
+ if err != nil {
+ t.Fatalf("open db: %v", err)
+ }
+ t.Cleanup(func() { d.Close() })
+ if _, err := migrate.Apply(context.Background(), d, BootSchema); err != nil {
+ t.Fatalf("migrate: %v", err)
+ }
+ a, err := NewAuth(d.Writer(), AuthEnv{
+ Origin: "https://sheets.example.com",
+ InstanceKey: "test-instance-key",
+ Owners: []string{"paul@example.com"},
+ Mailer: nil, // auth falls back to a logging sender; no mail is sent in this test
+ }, quietLogger())
+ if err != nil {
+ t.Fatalf("NewAuth: %v", err)
+ }
+ return New(d.G, a)
+}
+
+// TestAuthRoutesAreMounted proves the four handlers are reachable. It
+// exists because Router mounts them behind `if a.auth != nil`, and every
+// other test in this package passes nil — so without this, the whole
+// sign-in surface could be unmounted and the suite would stay green.
+func TestAuthRoutesAreMounted(t *testing.T) {
+ app := testAuth(t)
+ for _, tc := range []struct{ method, path string }{
+ {http.MethodPost, "/signin"},
+ {http.MethodGet, "/auth/callback"},
+ {http.MethodGet, "/auth/verify"},
+ {http.MethodPost, "/signout"},
+ } {
+ rec := httptest.NewRecorder()
+ app.Router().ServeHTTP(rec, httptest.NewRequest(tc.method, tc.path, nil))
+ if rec.Code == http.StatusNotFound || rec.Code == http.StatusMethodNotAllowed {
+ t.Errorf("%s %s = %d, handler not mounted", tc.method, tc.path, rec.Code)
+ }
+ }
+}
+
+// TestAuthRoutesAbsentWithoutAuth is the control for the test above: it
+// pins that those codes really do mean "not mounted", so the assertion
+// there is measuring what it claims to.
+func TestAuthRoutesAbsentWithoutAuth(t *testing.T) {
+ app := New(nil, nil)
+ for _, tc := range []struct{ method, path string }{
+ {http.MethodPost, "/signin"},
+ {http.MethodGet, "/auth/verify"},
+ } {
+ rec := httptest.NewRecorder()
+ app.Router().ServeHTTP(rec, httptest.NewRequest(tc.method, tc.path, nil))
+ if rec.Code != http.StatusNotFound && rec.Code != http.StatusMethodNotAllowed {
+ t.Errorf("%s %s = %d with nil auth, want 404/405", tc.method, tc.path, rec.Code)
+ }
+ }
+}