krz/gitbay

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
@@ -160,9 +160,11 @@ func serveCmd() *cobra.Command {
160160 ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
161161 defer stop()
162162
163 // Outbound webhook deliveries. The retry base is overridable
164 // for tests via GITBAY_WEBHOOK_RETRY_BASE.
165 retryBase := 30 * time.Second
163 // Outbound webhook deliveries, and the mail queue when SMTP is
164 // configured, share one retry base. It is overridable for
165 // tests via GITBAY_WEBHOOK_RETRY_BASE; notify.DefaultRetryBase
166 // names the production default so nothing else has to guess it.
167 retryBase := notify.DefaultRetryBase
166168 if v := os.Getenv("GITBAY_WEBHOOK_RETRY_BASE"); v != "" {
167169 if d, err := time.ParseDuration(v); err == nil {
168170 retryBase = d
e2e/emaillogin_test.go +58 −4
@@ -57,7 +57,9 @@ func TestEmailLogin(t *testing.T) {
5757 if status != 200 || !strings.Contains(body, "on its way") {
5858 t.Fatalf("POST /login by username: %d", status)
5959 }
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)
6163 for time.Now().Before(deadline) && len(smtp.mailTo("dana@example.test")) < 2 {
6264 time.Sleep(25 * time.Millisecond)
6365 }
@@ -66,6 +68,43 @@ func TestEmailLogin(t *testing.T) {
6668 }
6769}
6870
71// A verified secondary address stands in for an unverified primary:
72// resolution by username must not stop at the primary (#158).
73func 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
69108// The response must not say whether an account exists. A different status,
70109// body, or destination answers "is this person here?" to anyone who asks.
71110func TestEmailLoginDoesNotEnumerate(t *testing.T) {
@@ -78,6 +117,18 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
78117 // An account whose address was never verified must look like an absent
79118 // one, or an unverified address becomes an oracle.
80119 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 }
81132
82133 browser := newBrowser(t)
83134 real1, bodyReal := browserPost(t, browser, inst.base()+"/login",
@@ -95,6 +146,8 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
95146 url.Values{"identifier": {""}})
96147 absentUser, bodyAbsentUser := browserPost(t, browser, inst.base()+"/login",
97148 url.Values{"identifier": {"nosuchuser"}})
149 unverPrimary, bodyUnverPrimary := browserPost(t, browser, inst.base()+"/login",
150 url.Values{"identifier": {"frank"}})
98151
99152 for _, c := range []struct {
100153 name string
@@ -105,6 +158,7 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
105158 {"unverified", unver, bodyUnver},
106159 {"empty", empty, bodyEmpty},
107160 {"absent-username", absentUser, bodyAbsentUser},
161 {"unverified-primary-verified-secondary", unverPrimary, bodyUnverPrimary},
108162 } {
109163 if c.status != real1 || c.body != bodyReal {
110164 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) {
146200 t.Error("the throttled response differs from the first")
147201 }
148202
149 // Mail goes out from a goroutine, not on the request path, so give the
150 // last permitted one time to land before counting.
203 // Mail goes out through the notification queue, not on the request
204 // path, so give the last permitted one time to land before counting.
151205 var n int
152 deadline := time.Now().Add(2 * time.Second)
206 deadline := time.Now().Add(5 * time.Second)
153207 for time.Now().Before(deadline) {
154208 n = len(smtp.mailTo("dana@example.test"))
155209 if n >= 5 {
internal/control/loginlink.go +10 −14
@@ -2,12 +2,10 @@ package control
22
33import (
44 "fmt"
5 "log/slog"
65 "strings"
76 "time"
87
98 "gitbay.org/gitbay/internal/config"
10 "gitbay.org/gitbay/internal/mail"
119 "gitbay.org/gitbay/internal/store"
1210)
1311
@@ -63,7 +61,7 @@ func RequestLoginLink(cfg config.Config, st *store.Store, identifier string) err
6361 if err != nil {
6462 return nil
6563 }
66 addr, err := st.PrimaryVerifiedEmail(u.ID)
64 addr, err := st.PreferredVerifiedEmail(u.ID)
6765 if err != nil || addr == "" {
6866 return nil
6967 }
@@ -100,15 +98,13 @@ func RequestLoginLink(cfg config.Config, st *store.Store, identifier string) err
10098 host, strings.TrimSuffix(cfg.Server.SiteURL, "/"), token)
10199 subject := "log in to " + host
102100
103 // Sent in the background: mail.Send is a synchronous SMTP round trip to
104 // the relay, tens to hundreds of milliseconds against the sub-millisecond
105 // a miss takes to answer. Returning before it completes keeps every case
106 // — hit, miss, unverified, throttled — on the same DB-bound path, so
107 // response time cannot answer what the response body is built not to.
108 go func() {
109 if err := mail.Send(cfg, address, subject, body); err != nil {
110 slog.Error("login link mail", "user", user.ID, "err", err)
111 }
112 }()
113 return nil
101 // Queued rather than sent inline: the INSERT is sub-millisecond, the
102 // same order of cost as the miss path's SELECT, so every case — hit,
103 // miss, unverified, throttled — still resolves on the same DB-bound
104 // path. notify.Mailer drains the queue with retries (30s, 60s, 120s,
105 // 240s, then dead-lettered) that top out at 450s, comfortably inside
106 // the 15-minute link TTL, so a retried delivery cannot outlive the
107 // link it carries. Unlike the goroutine this replaces, a crash mid
108 // delivery does not lose the mail.
109 return st.EnqueueMail(address, subject, body)
114110}
internal/control/loginlink_test.go added +31
@@ -0,0 +1,31 @@
1package control
2
3import (
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.
20func 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
@@ -13,6 +13,16 @@ import (
1313 "gitbay.org/gitbay/internal/store"
1414)
1515
16// DefaultMaxAttempts and DefaultRetryBase are the retry parameters gitbayd
17// wires up in production (cmd/gitbayd/main.go), named so anything that
18// needs to reason about the mailer's worst-case delivery time — such as
19// checking it against a login link's TTL — computes it from the numbers
20// actually in force rather than a copy of them.
21const (
22 DefaultMaxAttempts = 5
23 DefaultRetryBase = 30 * time.Second
24)
25
1626type Mailer struct {
1727 St *store.Store
1828 Cfg config.Config
@@ -21,7 +31,7 @@ type Mailer struct {
2131}
2232
2333func New(st *store.Store, cfg config.Config, retryBase time.Duration) *Mailer {
24 return &Mailer{St: st, Cfg: cfg, RetryBase: retryBase, MaxAttempts: 5}
34 return &Mailer{St: st, Cfg: cfg, RetryBase: retryBase, MaxAttempts: DefaultMaxAttempts}
2535}
2636
2737// 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) {
447447 }
448448 return addr, err
449449}
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.
456func (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) {
7171 t.Fatalf("imported merge stamp: %+v", mr)
7272 }
7373}
74
75// PreferredVerifiedEmail falls back to a verified secondary when the
76// primary is not verified, unlike PrimaryVerifiedEmail (#158).
77func 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.
123func 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}