Login link: queue the mail, resolve any verified address !270

merged merged by cmc on 2026-09-05 19:51 UTC · krz/gitbay:login-followups into main

7 files changed, +203 −22

Layout: unified · split

cmd/gitbayd/main.go +5 −3

 			ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
 			defer stop()
-			// Outbound webhook deliveries. The retry base is overridable
+			// Outbound webhook deliveries, and the mail queue when SMTP is
-			// for tests via GITBAY_WEBHOOK_RETRY_BASE.
+			// configured, share one retry base. It is overridable for
-			retryBase := 30 * time.Second
+			// tests via GITBAY_WEBHOOK_RETRY_BASE; notify.DefaultRetryBase
+			// names the production default so nothing else has to guess it.
+			retryBase := notify.DefaultRetryBase
 			if v := os.Getenv("GITBAY_WEBHOOK_RETRY_BASE"); v != "" {
 				if d, err := time.ParseDuration(v); err == nil {
 					retryBase = d

e2e/emaillogin_test.go +58 −4

	@@ -57,7 +57,9 @@ func TestEmailLogin(t *testing.T) {
57	if status != 200 \|\| !strings.Contains(body, "on its way") {	57	if status != 200 \|\| !strings.Contains(body, "on its way") {
58	t.Fatalf("POST /login by username: %d", status)	58	t.Fatalf("POST /login by username: %d", status)
59	}	59	}
60	deadline := time.Now().Add(2 * time.Second)	60	// Mail goes out through the notification queue, not on the request
		61	// path, so give the mailer's poll tick time to pick it up.
		62	deadline := time.Now().Add(5 * time.Second)
61	for time.Now().Before(deadline) && len(smtp.mailTo("dana@example.test")) < 2 {	63	for time.Now().Before(deadline) && len(smtp.mailTo("dana@example.test")) < 2 {
62	time.Sleep(25 * time.Millisecond)	64	time.Sleep(25 * time.Millisecond)
63	}	65	}
	@@ -66,6 +68,43 @@ func TestEmailLogin(t *testing.T) {
66	}	68	}
67	}	69	}
68		70
		71	// A verified secondary address stands in for an unverified primary:
		72	// resolution by username must not stop at the primary (#158).
		73	func TestEmailLoginResolvesVerifiedSecondary(t *testing.T) {
		74	smtp := startFakeSMTP(t)
		75	inst := startInstanceWith(t, fmt.Sprintf(
		76	"[web]\nmode = \"accounts\"\n[mail]\nsmtp_host = %q\nfrom = \"noreply@gitbay.test\"\n",
		77	smtp.addr))
		78
		79	key := inst.newKey(t, "gus")
		80	inst.admin(t, "admin", "user", "create", "gus", "--key", key+".pub",
		81	"--email", "gus@primary.test") // primary added, left unverified
		82	if _, errOut, code := inst.ssh(t, key, "", "email", "add", "gus@secondary.test"); code != 0 {
		83	t.Fatalf("email add: exit %d %s", code, errOut)
		84	}
		85	verifyCode := extractCode(t, smtp.waitMail(t, 0))
		86	if _, errOut, code := inst.ssh(t, key, "", "email", "verify", verifyCode); code != 0 {
		87	t.Fatalf("email verify: exit %d %s", code, errOut)
		88	}
		89
		90	browser := newBrowser(t)
		91	status, body := browserPost(t, browser, inst.base()+"/login", url.Values{"identifier": {"gus"}})
		92	if status != 200 \|\| !strings.Contains(body, "on its way") {
		93	t.Fatalf("POST /login by username with an unverified primary: %d %s", status, body)
		94	}
		95
		96	link := loginLinkIn(smtp.waitFor(t, "gus@secondary.test", "/login?token="))
		97	if status, _ := browserGet(t, browser, inst.base()+link); status != 200 {
		98	t.Fatalf("following the link: %d", status)
		99	}
		100	if _, body := browserGet(t, browser, inst.base()+"/settings"); !strings.Contains(body, "gus@secondary.test") {
		101	t.Fatal("not logged in via the verified secondary address")
		102	}
		103	if len(smtp.mailTo("gus@primary.test")) != 0 {
		104	t.Error("mailed the unverified primary")
		105	}
		106	}
		107
69	// The response must not say whether an account exists. A different status,	108	// The response must not say whether an account exists. A different status,
70	// body, or destination answers "is this person here?" to anyone who asks.	109	// body, or destination answers "is this person here?" to anyone who asks.
71	func TestEmailLoginDoesNotEnumerate(t *testing.T) {	110	func TestEmailLoginDoesNotEnumerate(t *testing.T) {
	@@ -78,6 +117,18 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
78	// An account whose address was never verified must look like an absent	117	// An account whose address was never verified must look like an absent
79	// one, or an unverified address becomes an oracle.	118	// one, or an unverified address becomes an oracle.
80	inst.admin(t, "admin", "user", "create", "eve", "--email", "eve@example.test")	119	inst.admin(t, "admin", "user", "create", "eve", "--email", "eve@example.test")
		120	// An unverified primary with a verified secondary resolves the same as
		121	// a normal hit (#158) — this must be indistinguishable too.
		122	frankKey := inst.newKey(t, "frank")
		123	inst.admin(t, "admin", "user", "create", "frank", "--key", frankKey+".pub",
		124	"--email", "frank@example.test")
		125	if _, errOut, code := inst.ssh(t, frankKey, "", "email", "add", "frank2@example.test"); code != 0 {
		126	t.Fatalf("email add: exit %d %s", code, errOut)
		127	}
		128	if _, errOut, code := inst.ssh(t, frankKey, "", "email", "verify",
		129	extractCode(t, smtp.waitMail(t, 0))); code != 0 {
		130	t.Fatalf("email verify: exit %d %s", code, errOut)
		131	}
81		132
82	browser := newBrowser(t)	133	browser := newBrowser(t)
83	real1, bodyReal := browserPost(t, browser, inst.base()+"/login",	134	real1, bodyReal := browserPost(t, browser, inst.base()+"/login",
	@@ -95,6 +146,8 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
95	url.Values{"identifier": {""}})	146	url.Values{"identifier": {""}})
96	absentUser, bodyAbsentUser := browserPost(t, browser, inst.base()+"/login",	147	absentUser, bodyAbsentUser := browserPost(t, browser, inst.base()+"/login",
97	url.Values{"identifier": {"nosuchuser"}})	148	url.Values{"identifier": {"nosuchuser"}})
		149	unverPrimary, bodyUnverPrimary := browserPost(t, browser, inst.base()+"/login",
		150	url.Values{"identifier": {"frank"}})
98		151
99	for _, c := range []struct {	152	for _, c := range []struct {
100	name string	153	name string
	@@ -105,6 +158,7 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
105	{"unverified", unver, bodyUnver},	158	{"unverified", unver, bodyUnver},
106	{"empty", empty, bodyEmpty},	159	{"empty", empty, bodyEmpty},
107	{"absent-username", absentUser, bodyAbsentUser},	160	{"absent-username", absentUser, bodyAbsentUser},
		161	{"unverified-primary-verified-secondary", unverPrimary, bodyUnverPrimary},
108	} {	162	} {
109	if c.status != real1 \|\| c.body != bodyReal {	163	if c.status != real1 \|\| c.body != bodyReal {
110	t.Errorf("%s differs from a real address: status %d vs %d", c.name, c.status, real1)	164	t.Errorf("%s differs from a real address: status %d vs %d", c.name, c.status, real1)
	@@ -146,10 +200,10 @@ func TestEmailLoginThrottled(t *testing.T) {
146	t.Error("the throttled response differs from the first")	200	t.Error("the throttled response differs from the first")
147	}	201	}
148		202
149	// Mail goes out from a goroutine, not on the request path, so give the	203	// Mail goes out through the notification queue, not on the request
150	// last permitted one time to land before counting.	204	// path, so give the last permitted one time to land before counting.
151	var n int	205	var n int
152	deadline := time.Now().Add(2 * time.Second)	206	deadline := time.Now().Add(5 * time.Second)
153	for time.Now().Before(deadline) {	207	for time.Now().Before(deadline) {
154	n = len(smtp.mailTo("dana@example.test"))	208	n = len(smtp.mailTo("dana@example.test"))
155	if n >= 5 {	209	if n >= 5 {

internal/control/loginlink.go +10 −14

 import (
 	"fmt"
-	"log/slog"
 	"strings"
 	"time"
 	"gitbay.org/gitbay/internal/config"
-	"gitbay.org/gitbay/internal/mail"
 	"gitbay.org/gitbay/internal/store"
 )
 		if err != nil {
 			return nil
 		}
-		addr, err := st.PrimaryVerifiedEmail(u.ID)
+		addr, err := st.PreferredVerifiedEmail(u.ID)
 		if err != nil || addr == "" {
 			return nil
 		}
 		host, strings.TrimSuffix(cfg.Server.SiteURL, "/"), token)
 	subject := "log in to " + host
-	// Sent in the background: mail.Send is a synchronous SMTP round trip to
+	// Queued rather than sent inline: the INSERT is sub-millisecond, the
-	// the relay, tens to hundreds of milliseconds against the sub-millisecond
+	// same order of cost as the miss path's SELECT, so every case — hit,
-	// a miss takes to answer. Returning before it completes keeps every case
+	// miss, unverified, throttled — still resolves on the same DB-bound
-	// — hit, miss, unverified, throttled — on the same DB-bound path, so
+	// path. notify.Mailer drains the queue with retries (30s, 60s, 120s,
-	// response time cannot answer what the response body is built not to.
+	// 240s, then dead-lettered) that top out at 450s, comfortably inside
-	go func() {
+	// the 15-minute link TTL, so a retried delivery cannot outlive the
-		if err := mail.Send(cfg, address, subject, body); err != nil {
+	// link it carries. Unlike the goroutine this replaces, a crash mid
-			slog.Error("login link mail", "user", user.ID, "err", err)
+	// delivery does not lose the mail.
-		}
+	return st.EnqueueMail(address, subject, body)
-	}()
-	return nil
 }

internal/control/loginlink_test.go added +31

@@ -0,0 +1,31 @@
	1	package control
	2
	3	import (
	4	"testing"
	5	"time"
	6
	7	// Aliased: this package already has a top-level function named notify
	8	// (notifications.go), which the default import name would collide with.
	9	mailqueue "gitbay.org/gitbay/internal/notify"
	10	)
	11
	12	// Nothing ties notify's retry parameters to loginLinkTTL: someone tuning
	13	// mail retries for a slow relay has no reason to think about login links,
	14	// and the failure if they drift apart is silent — a link delivered after
	15	// it has already expired, refused with no indication why. This computes
	16	// the mailer's worst-case delivery time from notify's own named constants
	17	// (attempts 1..MaxAttempts-1 each wait RetryBase<<(attempt-1); the
	18	// MaxAttempts'th failure is dead-lettered immediately, per notify.Mailer.Run)
	19	// rather than a copy of the numbers, so either side moving breaks this test.
	20	func TestLoginLinkOutlivesMailerRetries(t *testing.T) {
	21	var worst time.Duration
	22	for attempt := 1; attempt < mailqueue.DefaultMaxAttempts; attempt++ {
	23	worst += mailqueue.DefaultRetryBase << (attempt - 1)
	24	}
	25	if worst >= loginLinkTTL {
	26	t.Fatalf("mailqueue.DefaultRetryBase=%s, mailqueue.DefaultMaxAttempts=%d: worst-case "+
	27	"delivery is %s, not less than loginLinkTTL=%s — a retried login link mail can "+
	28	"arrive after the link it carries has expired",
	29	mailqueue.DefaultRetryBase, mailqueue.DefaultMaxAttempts, worst, loginLinkTTL)
	30	}
	31	}

internal/notify/notify.go +11 −1

 	"gitbay.org/gitbay/internal/store"
 )
+// DefaultMaxAttempts and DefaultRetryBase are the retry parameters gitbayd
+// wires up in production (cmd/gitbayd/main.go), named so anything that
+// needs to reason about the mailer's worst-case delivery time — such as
+// checking it against a login link's TTL — computes it from the numbers
+// actually in force rather than a copy of them.
+const (
+	DefaultMaxAttempts = 5
+	DefaultRetryBase   = 30 * time.Second
+)
 type Mailer struct {
 	St          *store.Store
 	Cfg         config.Config
 }
 func New(st *store.Store, cfg config.Config, retryBase time.Duration) *Mailer {
-	return &Mailer{St: st, Cfg: cfg, RetryBase: retryBase, MaxAttempts: 5}
+	return &Mailer{St: st, Cfg: cfg, RetryBase: retryBase, MaxAttempts: DefaultMaxAttempts}
 }
 // Run polls for due mail until ctx is done.

internal/store/mrs.go +17

	@@ -447,3 +447,20 @@ func (s *Store) PrimaryVerifiedEmail(userID int64) (string, error) {
447	}	447	}
448	return addr, err	448	return addr, err
449	}	449	}
		450
		451	// PreferredVerifiedEmail returns the primary address if it is verified,
		452	// otherwise the account's other verified address that sorts first by
		453	// address; "" if none is verified. Unlike PrimaryVerifiedEmail, a verified
		454	// secondary counts: an account that verified one address but not its
		455	// primary still has somewhere to send a login link.
		456	func (s *Store) PreferredVerifiedEmail(userID int64) (string, error) {
		457	var addr string
		458	err := s.DB.QueryRow(
		459	`SELECT address FROM emails WHERE user_id = ? AND verified_at IS NOT NULL
		460	ORDER BY is_primary DESC, address LIMIT 1`,
		461	userID).Scan(&addr)
		462	if errors.Is(err, sql.ErrNoRows) {
		463	return "", nil
		464	}
		465	return addr, err
		466	}

internal/store/mrs_test.go +71

	@@ -71,3 +71,74 @@ func TestResolutionStampImported(t *testing.T) {
71	t.Fatalf("imported merge stamp: %+v", mr)	71	t.Fatalf("imported merge stamp: %+v", mr)
72	}	72	}
73	}	73	}
		74
		75	// PreferredVerifiedEmail falls back to a verified secondary when the
		76	// primary is not verified, unlike PrimaryVerifiedEmail (#158).
		77	func TestPreferredVerifiedEmail(t *testing.T) {
		78	s := open(t)
		79	if err := s.MigrateUp(); err != nil {
		80	t.Fatal(err)
		81	}
		82	uid, err := s.CreateUser("gus", false)
		83	if err != nil {
		84	t.Fatal(err)
		85	}
		86
		87	if addr, err := s.PreferredVerifiedEmail(uid); err != nil \|\| addr != "" {
		88	t.Fatalf("no addresses at all: %q, %v", addr, err)
		89	}
		90
		91	if err := s.AddEmail(uid, "primary@example.test", "", true); err != nil {
		92	t.Fatal(err)
		93	}
		94	if addr, err := s.PreferredVerifiedEmail(uid); err != nil \|\| addr != "" {
		95	t.Fatalf("unverified primary only: %q, %v", addr, err)
		96	}
		97	if addr, err := s.PrimaryVerifiedEmail(uid); err != nil \|\| addr != "" {
		98	t.Fatalf("PrimaryVerifiedEmail on an unverified primary: %q, %v", addr, err)
		99	}
		100
		101	if err := s.AddEmail(uid, "secondary@example.test", "smtp", false); err != nil {
		102	t.Fatal(err)
		103	}
		104	if addr, err := s.PreferredVerifiedEmail(uid); err != nil \|\| addr != "secondary@example.test" {
		105	t.Fatalf("unverified primary, verified secondary: %q, %v", addr, err)
		106	}
		107	// PrimaryVerifiedEmail keeps meaning exactly what it says: still "",
		108	// because the primary itself is still unverified.
		109	if addr, err := s.PrimaryVerifiedEmail(uid); err != nil \|\| addr != "" {
		110	t.Fatalf("PrimaryVerifiedEmail with only the secondary verified: %q, %v", addr, err)
		111	}
		112
		113	if err := s.VerifyEmail(uid, "primary@example.test", "admin"); err != nil {
		114	t.Fatal(err)
		115	}
		116	if addr, err := s.PreferredVerifiedEmail(uid); err != nil \|\| addr != "primary@example.test" {
		117	t.Fatalf("both verified, primary should win: %q, %v", addr, err)
		118	}
		119	}
		120
		121	// With no verified primary, the choice among verified secondaries must not
		122	// depend on insertion or row order.
		123	func TestPreferredVerifiedEmailDeterministicTiebreak(t *testing.T) {
		124	s := open(t)
		125	if err := s.MigrateUp(); err != nil {
		126	t.Fatal(err)
		127	}
		128	uid, err := s.CreateUser("gus", false)
		129	if err != nil {
		130	t.Fatal(err)
		131	}
		132	if err := s.AddEmail(uid, "primary@example.test", "", true); err != nil {
		133	t.Fatal(err)
		134	}
		135	if err := s.AddEmail(uid, "zzz@example.test", "smtp", false); err != nil {
		136	t.Fatal(err)
		137	}
		138	if err := s.AddEmail(uid, "aaa@example.test", "smtp", false); err != nil {
		139	t.Fatal(err)
		140	}
		141	if addr, err := s.PreferredVerifiedEmail(uid); err != nil \|\| addr != "aaa@example.test" {
		142	t.Fatalf("tiebreak should be alphabetical: %q, %v", addr, err)
		143	}
		144	}