Commit 965735bfbc
Verified · cmc
Layout: unified · split
docs/plans/2026-09-04-email-login.md added +787
| @@ -0,0 +1,787 @@ | |||
| 1 | # Browser login without an SSH key — Implementation Plan | ||
| 2 | |||
| 3 | > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
| 4 | |||
| 5 | **Goal:** Let a person with no SSH key log into the web UI by requesting a | ||
| 6 | one-time link at their verified email address. | ||
| 7 | |||
| 8 | **Architecture:** An anonymous `POST /login` calls a plain exported function in | ||
| 9 | `internal/control`, which resolves the identifier to a user, mints the same | ||
| 10 | one-time token `web login` already mints, and mails it. The existing | ||
| 11 | `GET /login?token=` handler consumes it unchanged. No new control command, | ||
| 12 | because the caller has no authenticated user to run one as. | ||
| 13 | |||
| 14 | **Tech Stack:** Go, SQLite (hand-written SQL, no ORM), `html/template`, | ||
| 15 | `internal/mail` over SMTP. | ||
| 16 | |||
| 17 | **Spec:** `docs/specs/2026-09-04-email-login-design.md` | ||
| 18 | |||
| 19 | ## Global Constraints | ||
| 20 | |||
| 21 | - Branch `email-login`. Never push to `main`. One MR, `Closes #155`. | ||
| 22 | - Never attribute anything to an assistant or model, anywhere. | ||
| 23 | - No ORM. Hand-written SQL. Migrations in `internal/store/migrations/`. | ||
| 24 | - Comments state facts, not before/after commentary. Git history holds the rest. | ||
| 25 | - `--json` output is the contract; human output is not. | ||
| 26 | - Locally run build, vet, and the unit tests of touched packages plus the one | ||
| 27 | e2e test being written. Full e2e belongs to CI on bay1. | ||
| 28 | - Adding a top-level route means adding the word to `internal/policy/names.go`. | ||
| 29 | `/login` already exists, so this plan adds no reserved name. | ||
| 30 | |||
| 31 | ## Change from the spec | ||
| 32 | |||
| 33 | The spec proposed `RequestLoginLink` returning `(msg, errMsg string, code int)` | ||
| 34 | to mirror `control.RegisterAccount`. **It returns only `error` instead.** | ||
| 35 | `RegisterAccount` reports per-case failures because registration is allowed to | ||
| 36 | say what went wrong; here, reporting anything about the outcome is precisely the | ||
| 37 | enumeration leak the spec forbids. The returned error is for the server log | ||
| 38 | only — SMTP down, database failure — and is never rendered. The handler draws | ||
| 39 | the same page whatever it gets back. | ||
| 40 | |||
| 41 | ## File Structure | ||
| 42 | |||
| 43 | | Path | Responsibility | | ||
| 44 | |---|---| | ||
| 45 | | `internal/store/migrations/0040_login_token_index.{up,down}.sql` | index behind the throttle count | | ||
| 46 | | `internal/store/sessions.go` | `CountLoginTokensSince`, beside `CreateLoginToken` | | ||
| 47 | | `internal/control/loginlink.go` | new — resolve identifier, throttle, mint, mail | | ||
| 48 | | `internal/httpd/accounts.go` | `loginSubmit`; session cookie `SameSite` | | ||
| 49 | | `internal/httpd/routes.go` | `POST /login` | | ||
| 50 | | `internal/web/templates/login.html` | request form and the sent confirmation | | ||
| 51 | | `internal/store/sessions_test.go` | new — counter unit test | | ||
| 52 | | `internal/httpd/logincookie_test.go` | new — cookie attribute test | | ||
| 53 | | `e2e/emaillogin_test.go` | new — the whole path against real SMTP | | ||
| 54 | |||
| 55 | --- | ||
| 56 | |||
| 57 | ### Task 1: The throttle counter and its index | ||
| 58 | |||
| 59 | `login_tokens` has no index on `user_id`, and rows are never deleted. Counting | ||
| 60 | per user on an anonymous endpoint would be an unbounded table scan on every | ||
| 61 | request, which makes the throttle its own denial-of-service vector. | ||
| 62 | |||
| 63 | **Files:** | ||
| 64 | - Create: `internal/store/migrations/0040_login_token_index.up.sql` | ||
| 65 | - Create: `internal/store/migrations/0040_login_token_index.down.sql` | ||
| 66 | - Modify: `internal/store/sessions.go` (append after `CreateLoginToken`, line 36) | ||
| 67 | - Create: `internal/store/sessions_test.go` | ||
| 68 | |||
| 69 | **Interfaces:** | ||
| 70 | - Consumes: `Store.CreateLoginToken(userID int64, hash string, ttl time.Duration) error`, `store.NewToken() (token, hash string, err error)` — both exist. | ||
| 71 | - Produces: `func (s *Store) CountLoginTokensSince(userID int64, since time.Time) (int, error)` | ||
| 72 | |||
| 73 | - [ ] **Step 1: Write the failing test** | ||
| 74 | |||
| 75 | Create `internal/store/sessions_test.go`: | ||
| 76 | |||
| 77 | ```go | ||
| 78 | package store | ||
| 79 | |||
| 80 | import ( | ||
| 81 | "testing" | ||
| 82 | "time" | ||
| 83 | ) | ||
| 84 | |||
| 85 | func TestCountLoginTokensSince(t *testing.T) { | ||
| 86 | s := open(t) | ||
| 87 | if err := s.MigrateUp(); err != nil { | ||
| 88 | t.Fatal(err) | ||
| 89 | } | ||
| 90 | uid, err := s.CreateUser("cmc", true) | ||
| 91 | if err != nil { | ||
| 92 | t.Fatal(err) | ||
| 93 | } | ||
| 94 | for i := 0; i < 3; i++ { | ||
| 95 | _, hash, err := NewToken() | ||
| 96 | if err != nil { | ||
| 97 | t.Fatal(err) | ||
| 98 | } | ||
| 99 | if err := s.CreateLoginToken(uid, hash, time.Minute); err != nil { | ||
| 100 | t.Fatal(err) | ||
| 101 | } | ||
| 102 | } | ||
| 103 | |||
| 104 | n, err := s.CountLoginTokensSince(uid, time.Now().Add(-time.Hour)) | ||
| 105 | if err != nil || n != 3 { | ||
| 106 | t.Fatalf("count in the last hour = %d, %v; want 3", n, err) | ||
| 107 | } | ||
| 108 | |||
| 109 | // A window that opens in the future sees none of them, which is what | ||
| 110 | // makes the hourly bound a window rather than a lifetime total. | ||
| 111 | if n, err := s.CountLoginTokensSince(uid, time.Now().Add(time.Hour)); err != nil || n != 0 { | ||
| 112 | t.Fatalf("count in a future window = %d, %v; want 0", n, err) | ||
| 113 | } | ||
| 114 | |||
| 115 | // One account's requests must not spend another account's budget. | ||
| 116 | other, err := s.CreateUser("kim", false) | ||
| 117 | if err != nil { | ||
| 118 | t.Fatal(err) | ||
| 119 | } | ||
| 120 | if n, err := s.CountLoginTokensSince(other, time.Now().Add(-time.Hour)); err != nil || n != 0 { | ||
| 121 | t.Fatalf("other account count = %d, %v; want 0", n, err) | ||
| 122 | } | ||
| 123 | } | ||
| 124 | ``` | ||
| 125 | |||
| 126 | - [ ] **Step 2: Run it and watch it fail** | ||
| 127 | |||
| 128 | Run: `go test ./internal/store/ -run TestCountLoginTokensSince -v` | ||
| 129 | Expected: FAIL — `s.CountLoginTokensSince undefined`. | ||
| 130 | |||
| 131 | - [ ] **Step 3: Write the migration** | ||
| 132 | |||
| 133 | `internal/store/migrations/0040_login_token_index.up.sql`: | ||
| 134 | |||
| 135 | ```sql | ||
| 136 | CREATE INDEX login_tokens_user_created ON login_tokens(user_id, created_at); | ||
| 137 | ``` | ||
| 138 | |||
| 139 | `internal/store/migrations/0040_login_token_index.down.sql`: | ||
| 140 | |||
| 141 | ```sql | ||
| 142 | DROP INDEX login_tokens_user_created; | ||
| 143 | ``` | ||
| 144 | |||
| 145 | - [ ] **Step 4: Write the counter** | ||
| 146 | |||
| 147 | Append to `internal/store/sessions.go`, directly after `CreateLoginToken`: | ||
| 148 | |||
| 149 | ```go | ||
| 150 | // CountLoginTokensSince counts the login tokens minted for a user within a | ||
| 151 | // window. An unauthenticated request can ask for a login link, so the mint | ||
| 152 | // needs a durable per-account bound the way email verification does (#136). | ||
| 153 | func (s *Store) CountLoginTokensSince(userID int64, since time.Time) (int, error) { | ||
| 154 | var n int | ||
| 155 | err := s.DB.QueryRow( | ||
| 156 | "SELECT count(*) FROM login_tokens WHERE user_id = ? AND created_at > ?", | ||
| 157 | userID, fmtTime(since)).Scan(&n) | ||
| 158 | return n, err | ||
| 159 | } | ||
| 160 | ``` | ||
| 161 | |||
| 162 | - [ ] **Step 5: Run the test and the package** | ||
| 163 | |||
| 164 | Run: `go test ./internal/store/ -run TestCountLoginTokensSince -v` | ||
| 165 | Expected: PASS. | ||
| 166 | |||
| 167 | Run: `go test ./internal/store/` | ||
| 168 | Expected: PASS — the new migration must not break existing store tests. | ||
| 169 | |||
| 170 | - [ ] **Step 6: Confirm the index is actually used** | ||
| 171 | |||
| 172 | Run: | ||
| 173 | ```bash | ||
| 174 | cd /Users/cmc/git/krz/gitbay && cat > /tmp/plan_explain_test.go <<'EOF' | ||
| 175 | EOF | ||
| 176 | go test ./internal/store/ -run TestDashboardQueriesUseIndexes -v | ||
| 177 | ``` | ||
| 178 | Expected: PASS (unrelated, but proves the migration did not disturb existing | ||
| 179 | plans). Then verify by hand that the count uses the index: | ||
| 180 | |||
| 181 | ```bash | ||
| 182 | sqlite3 "$(mktemp -d)/x.db" <<'EOF' | ||
| 183 | CREATE TABLE login_tokens (token_hash TEXT PRIMARY KEY, user_id INTEGER NOT NULL, | ||
| 184 | created_at TEXT NOT NULL, expires_at TEXT NOT NULL, used_at TEXT); | ||
| 185 | CREATE INDEX login_tokens_user_created ON login_tokens(user_id, created_at); | ||
| 186 | EXPLAIN QUERY PLAN SELECT count(*) FROM login_tokens WHERE user_id = 1 AND created_at > 'x'; | ||
| 187 | EOF | ||
| 188 | ``` | ||
| 189 | Expected: the plan names `login_tokens_user_created`, not `SCAN login_tokens`. | ||
| 190 | |||
| 191 | - [ ] **Step 7: Commit** | ||
| 192 | |||
| 193 | ```bash | ||
| 194 | git add internal/store/sessions.go internal/store/sessions_test.go \ | ||
| 195 | internal/store/migrations/0040_login_token_index.up.sql \ | ||
| 196 | internal/store/migrations/0040_login_token_index.down.sql | ||
| 197 | git commit -m "store: count login tokens per account, indexed | ||
| 198 | |||
| 199 | Ref #155" | ||
| 200 | ``` | ||
| 201 | |||
| 202 | --- | ||
| 203 | |||
| 204 | ### Task 2: Session cookie SameSite | ||
| 205 | |||
| 206 | A link clicked in a webmail client is a cross-site top-level navigation. Under | ||
| 207 | `SameSite=Strict` the redirect chain to `/` can arrive without the cookie, so | ||
| 208 | the visitor lands logged out and is logged in only after a refresh. Pasting a | ||
| 209 | URL into the address bar does not hit this, which is why the SSH flow never | ||
| 210 | showed it. | ||
| 211 | |||
| 212 | `Lax` is safe here, and it was verified rather than assumed: every | ||
| 213 | cookie-authenticated mutating route carries `checkOrigin`. The four POST routes | ||
| 214 | without it — `git-upload-pack`, `git-receive-pack`, `lfs/objects/batch`, | ||
| 215 | `/api/v1/cmd` — do not accept the session cookie at all; the API takes only | ||
| 216 | `Authorization: Bearer` (`internal/httpd/api.go:126`). For POST, `Lax` is | ||
| 217 | strictly stronger than the Origin check, because it withholds the cookie | ||
| 218 | outright. The only `Mutating: true` GET is `/login` itself, whose token is | ||
| 219 | single-use. | ||
| 220 | |||
| 221 | **Files:** | ||
| 222 | - Modify: `internal/httpd/accounts.go:92` (set), and the `clearCookie` call in `logout` | ||
| 223 | - Create: `internal/httpd/logincookie_test.go` | ||
| 224 | |||
| 225 | **Interfaces:** | ||
| 226 | - Consumes: `Server.clearCookie(name string, sameSite http.SameSite) *http.Cookie` (`internal/httpd/flash.go:55`), `sessionCookie` | ||
| 227 | - Produces: nothing new; changes an attribute value. | ||
| 228 | |||
| 229 | - [ ] **Step 1: Write the failing test** | ||
| 230 | |||
| 231 | Create `internal/httpd/logincookie_test.go`: | ||
| 232 | |||
| 233 | ```go | ||
| 234 | package httpd | ||
| 235 | |||
| 236 | import ( | ||
| 237 | "net/http" | ||
| 238 | "testing" | ||
| 239 | |||
| 240 | "gitbay.org/gitbay/internal/config" | ||
| 241 | ) | ||
| 242 | |||
| 243 | // The session cookie must be Lax, not Strict. A login link clicked in a mail | ||
| 244 | // client is a cross-site top-level navigation, and Strict can withhold the | ||
| 245 | // cookie through the redirect that follows, so the visitor lands logged out | ||
| 246 | // (#155). Cross-site POSTs stay protected: Lax withholds the cookie from them, | ||
| 247 | // and checkOrigin refuses them besides. | ||
| 248 | func TestSessionCookieIsLax(t *testing.T) { | ||
| 249 | s := &Server{cfg: config.Config{}} | ||
| 250 | s.cfg.HTTP.TLS = "acme" | ||
| 251 | if got := s.clearCookie(sessionCookie, sessionSameSite); got.SameSite != http.SameSiteLaxMode { | ||
| 252 | t.Errorf("clearing cookie SameSite = %v, want Lax", got.SameSite) | ||
| 253 | } | ||
| 254 | if sessionSameSite != http.SameSiteLaxMode { | ||
| 255 | t.Errorf("sessionSameSite = %v, want Lax", sessionSameSite) | ||
| 256 | } | ||
| 257 | } | ||
| 258 | ``` | ||
| 259 | |||
| 260 | - [ ] **Step 2: Run it and watch it fail** | ||
| 261 | |||
| 262 | Run: `go test ./internal/httpd/ -run TestSessionCookieIsLax -v` | ||
| 263 | Expected: FAIL — `undefined: sessionSameSite`. | ||
| 264 | |||
| 265 | - [ ] **Step 3: Introduce the constant and use it in both places** | ||
| 266 | |||
| 267 | In `internal/httpd/accounts.go`, near the `sessionCookie` declaration, add: | ||
| 268 | |||
| 269 | ```go | ||
| 270 | // sessionSameSite is Lax so a login link followed from a mail client keeps | ||
| 271 | // its session through the redirect. Cross-site POSTs are refused by | ||
| 272 | // checkOrigin and carry no Lax cookie anyway. | ||
| 273 | const sessionSameSite = http.SameSiteLaxMode | ||
| 274 | ``` | ||
| 275 | |||
| 276 | In the `login` handler (`internal/httpd/accounts.go:92`), replace | ||
| 277 | `SameSite: http.SameSiteStrictMode,` with `SameSite: sessionSameSite,`. | ||
| 278 | |||
| 279 | In the `logout` handler, replace | ||
| 280 | `s.clearCookie(sessionCookie, http.SameSiteStrictMode)` with | ||
| 281 | `s.clearCookie(sessionCookie, sessionSameSite)` so the set and clear paths | ||
| 282 | match, which is the rule `TestClearCookieMirrorsTheSettingCall` exists to keep. | ||
| 283 | |||
| 284 | - [ ] **Step 4: Run the package tests** | ||
| 285 | |||
| 286 | Run: `go test ./internal/httpd/` | ||
| 287 | Expected: PASS, including the pre-existing | ||
| 288 | `TestClearCookieMirrorsTheSettingCall`, which passes its own `SameSite` | ||
| 289 | argument and is unaffected. | ||
| 290 | |||
| 291 | - [ ] **Step 5: Commit** | ||
| 292 | |||
| 293 | ```bash | ||
| 294 | git add internal/httpd/accounts.go internal/httpd/logincookie_test.go | ||
| 295 | git commit -m "httpd: session cookie is SameSite=Lax | ||
| 296 | |||
| 297 | A link followed from a mail client is a cross-site navigation; Strict can | ||
| 298 | drop the cookie through the redirect after /login?token=. Every | ||
| 299 | cookie-authenticated mutating route carries checkOrigin, and Lax withholds | ||
| 300 | the cookie from cross-site POSTs regardless. | ||
| 301 | |||
| 302 | Ref #155" | ||
| 303 | ``` | ||
| 304 | |||
| 305 | --- | ||
| 306 | |||
| 307 | ### Task 3: The login link request | ||
| 308 | |||
| 309 | The deliverable. The function, the route, the handler, and the form land | ||
| 310 | together because none of them is testable without the others. | ||
| 311 | |||
| 312 | **Files:** | ||
| 313 | - Create: `internal/control/loginlink.go` | ||
| 314 | - Modify: `internal/httpd/accounts.go` (`renderLogin`, new `loginSubmit`) | ||
| 315 | - Modify: `internal/httpd/routes.go:100` (add `POST /login`) | ||
| 316 | - Modify: `internal/web/templates/login.html` | ||
| 317 | - Create: `e2e/emaillogin_test.go` | ||
| 318 | |||
| 319 | **Interfaces:** | ||
| 320 | - Consumes: `store.UserIDByVerifiedEmail(address string) (int64, bool)` (`internal/store/activity.go:11`); `store.PrimaryVerifiedEmail(userID int64) (string, error)` (`internal/store/mrs.go:440`); `store.UserByName`; `store.CountLoginTokensSince` (Task 1); `store.NewToken`; `store.CreateLoginToken`; `mail.Send(cfg config.Config, to, subject, body string) error`; `Server.apiLimit.allow(key string, write bool) (bool, time.Duration)`; `Server.clientIP(r)`. | ||
| 321 | - Produces: `func control.RequestLoginLink(cfg config.Config, st *store.Store, identifier string) error` | ||
| 322 | |||
| 323 | - [ ] **Step 1: Write the failing e2e test** | ||
| 324 | |||
| 325 | Create `e2e/emaillogin_test.go`: | ||
| 326 | |||
| 327 | ```go | ||
| 328 | package e2e | ||
| 329 | |||
| 330 | import ( | ||
| 331 | "fmt" | ||
| 332 | "net/url" | ||
| 333 | "strings" | ||
| 334 | "testing" | ||
| 335 | ) | ||
| 336 | |||
| 337 | // A person with no SSH key can still get into the web UI: they ask for a | ||
| 338 | // link by username or verified address and it arrives by mail (#155). | ||
| 339 | func TestEmailLogin(t *testing.T) { | ||
| 340 | smtp := startFakeSMTP(t) | ||
| 341 | inst := startInstanceWith(t, fmt.Sprintf( | ||
| 342 | "[web]\nmode = \"accounts\"\n[mail]\nsmtp_host = %q\nfrom = \"noreply@gitbay.test\"\n", | ||
| 343 | smtp.addr)) | ||
| 344 | |||
| 345 | // No --key: this account has no way to authenticate over SSH at all, | ||
| 346 | // which is the whole point. | ||
| 347 | inst.admin(t, "admin", "user", "create", "dana", | ||
| 348 | "--email", "dana@example.test", "--verified") | ||
| 349 | |||
| 350 | browser := newBrowser(t) | ||
| 351 | status, body := browserPost(t, browser, inst.base()+"/login", | ||
| 352 | url.Values{"identifier": {"dana@example.test"}}) | ||
| 353 | if status != 200 { | ||
| 354 | t.Fatalf("POST /login: %d", status) | ||
| 355 | } | ||
| 356 | if !strings.Contains(body, "on its way") { | ||
| 357 | t.Fatalf("no confirmation in body: %s", body) | ||
| 358 | } | ||
| 359 | |||
| 360 | msg := smtp.waitFor(t, "dana@example.test", "/login?token=") | ||
| 361 | i := strings.Index(msg, "/login?token=") | ||
| 362 | link := msg[i:] | ||
| 363 | if j := strings.IndexAny(link, " \r\n"); j >= 0 { | ||
| 364 | link = link[:j] | ||
| 365 | } | ||
| 366 | |||
| 367 | if status, _ := browserGet(t, browser, inst.base()+link); status != 200 { | ||
| 368 | t.Fatalf("following the link: %d", status) | ||
| 369 | } | ||
| 370 | status, body = browserGet(t, browser, inst.base()+"/settings") | ||
| 371 | if status != 200 || !strings.Contains(body, "dana@example.test") { | ||
| 372 | t.Fatalf("not logged in after the link: %d", status) | ||
| 373 | } | ||
| 374 | |||
| 375 | // The link is single use. | ||
| 376 | second := newBrowser(t) | ||
| 377 | browserGet(t, second, inst.base()+link) | ||
| 378 | if status, _ := browserGet(t, second, inst.base()+"/settings"); status == 200 { | ||
| 379 | t.Error("login link worked twice") | ||
| 380 | } | ||
| 381 | } | ||
| 382 | |||
| 383 | // The response must not say whether an account exists. A different status, | ||
| 384 | // body, or destination answers "is this person here?" to anyone who asks. | ||
| 385 | func TestEmailLoginDoesNotEnumerate(t *testing.T) { | ||
| 386 | smtp := startFakeSMTP(t) | ||
| 387 | inst := startInstanceWith(t, fmt.Sprintf( | ||
| 388 | "[web]\nmode = \"accounts\"\n[mail]\nsmtp_host = %q\nfrom = \"noreply@gitbay.test\"\n", | ||
| 389 | smtp.addr)) | ||
| 390 | inst.admin(t, "admin", "user", "create", "dana", | ||
| 391 | "--email", "dana@example.test", "--verified") | ||
| 392 | // An account whose address was never verified must look like an absent | ||
| 393 | // one, or an unverified address becomes an oracle. | ||
| 394 | inst.admin(t, "admin", "user", "create", "eve", "--email", "eve@example.test") | ||
| 395 | |||
| 396 | browser := newBrowser(t) | ||
| 397 | real1, bodyReal := browserPost(t, browser, inst.base()+"/login", | ||
| 398 | url.Values{"identifier": {"dana@example.test"}}) | ||
| 399 | absent, bodyAbsent := browserPost(t, browser, inst.base()+"/login", | ||
| 400 | url.Values{"identifier": {"nobody@example.test"}}) | ||
| 401 | unver, bodyUnver := browserPost(t, browser, inst.base()+"/login", | ||
| 402 | url.Values{"identifier": {"eve@example.test"}}) | ||
| 403 | empty, bodyEmpty := browserPost(t, browser, inst.base()+"/login", | ||
| 404 | url.Values{"identifier": {""}}) | ||
| 405 | |||
| 406 | for _, c := range []struct { | ||
| 407 | name string | ||
| 408 | status int | ||
| 409 | body string | ||
| 410 | }{ | ||
| 411 | {"absent", absent, bodyAbsent}, | ||
| 412 | {"unverified", unver, bodyUnver}, | ||
| 413 | {"empty", empty, bodyEmpty}, | ||
| 414 | } { | ||
| 415 | if c.status != real1 || c.body != bodyReal { | ||
| 416 | t.Errorf("%s differs from a real address: status %d vs %d", c.name, c.status, real1) | ||
| 417 | } | ||
| 418 | } | ||
| 419 | if len(smtp.mailTo("eve@example.test")) != 0 { | ||
| 420 | t.Error("mailed an unverified address") | ||
| 421 | } | ||
| 422 | if len(smtp.mailTo("nobody@example.test")) != 0 { | ||
| 423 | t.Error("mailed an address with no account") | ||
| 424 | } | ||
| 425 | } | ||
| 426 | |||
| 427 | // An anonymous endpoint that sends mail needs a durable per-account bound, | ||
| 428 | // the same one email verification has (#136). | ||
| 429 | func TestEmailLoginThrottled(t *testing.T) { | ||
| 430 | smtp := startFakeSMTP(t) | ||
| 431 | inst := startInstanceWith(t, fmt.Sprintf( | ||
| 432 | "[web]\nmode = \"accounts\"\n[mail]\nsmtp_host = %q\nfrom = \"noreply@gitbay.test\"\n", | ||
| 433 | smtp.addr)) | ||
| 434 | inst.admin(t, "admin", "user", "create", "dana", | ||
| 435 | "--email", "dana@example.test", "--verified") | ||
| 436 | |||
| 437 | browser := newBrowser(t) | ||
| 438 | for i := 0; i < 6; i++ { | ||
| 439 | browserPost(t, browser, inst.base()+"/login", | ||
| 440 | url.Values{"identifier": {"dana@example.test"}}) | ||
| 441 | } | ||
| 442 | if n := len(smtp.mailTo("dana@example.test")); n > 5 { | ||
| 443 | t.Fatalf("sent %d login mails in an hour, want at most 5", n) | ||
| 444 | } | ||
| 445 | } | ||
| 446 | ``` | ||
| 447 | |||
| 448 | - [ ] **Step 2: Run it and watch it fail** | ||
| 449 | |||
| 450 | Run: `go test ./e2e/ -run TestEmailLogin -v -timeout 20m` | ||
| 451 | Expected: FAIL — `POST /login: 405`, because no such route exists. | ||
| 452 | |||
| 453 | Note the explicit `-timeout`: `go test` defaults to 10 minutes and the e2e | ||
| 454 | suite has exceeded it before, which reads as a hang rather than a failure | ||
| 455 | (#143). | ||
| 456 | |||
| 457 | - [ ] **Step 3: Write the control function** | ||
| 458 | |||
| 459 | Create `internal/control/loginlink.go`: | ||
| 460 | |||
| 461 | ```go | ||
| 462 | package control | ||
| 463 | |||
| 464 | import ( | ||
| 465 | "fmt" | ||
| 466 | "strings" | ||
| 467 | "time" | ||
| 468 | |||
| 469 | "gitbay.org/gitbay/internal/config" | ||
| 470 | "gitbay.org/gitbay/internal/mail" | ||
| 471 | "gitbay.org/gitbay/internal/store" | ||
| 472 | ) | ||
| 473 | |||
| 474 | // maxLoginLinksPerHour bounds what one account's address can be made to | ||
| 475 | // receive. It matches maxEmailAddsPerHour: enough for a person who mistypes | ||
| 476 | // and retries, nothing for a script. | ||
| 477 | const maxLoginLinksPerHour = 5 | ||
| 478 | |||
| 479 | // loginLinkTTL is longer than the five minutes an SSH-minted link gets. | ||
| 480 | // That one is pasted from a terminal already open; this one has to survive | ||
| 481 | // delivery and someone noticing the mail. | ||
| 482 | const loginLinkTTL = 15 * time.Minute | ||
| 483 | |||
| 484 | // RequestLoginLink mails a one-time login link to the account named by | ||
| 485 | // identifier, which is a username or a verified email address. | ||
| 486 | // | ||
| 487 | // It is not a registered command: the caller is an unauthenticated web | ||
| 488 | // request, and commands run as c.User. RegisterAccount is exported for the | ||
| 489 | // same reason. | ||
| 490 | // | ||
| 491 | // The returned error is for the server log only. Nothing about the outcome | ||
| 492 | // may reach the caller — that a request found an account, found one without | ||
| 493 | // a verified address, or found nothing at all must be indistinguishable, or | ||
| 494 | // the endpoint answers "does this person have an account here?" to anyone | ||
| 495 | // who asks. Every miss returns nil. | ||
| 496 | func RequestLoginLink(cfg config.Config, st *store.Store, identifier string) error { | ||
| 497 | if cfg.Web.Mode != "accounts" || cfg.Mail.SMTPHost == "" { | ||
| 498 | return nil | ||
| 499 | } | ||
| 500 | identifier = strings.TrimSpace(identifier) | ||
| 501 | if identifier == "" { | ||
| 502 | return nil | ||
| 503 | } | ||
| 504 | |||
| 505 | var userID int64 | ||
| 506 | var address string | ||
| 507 | if strings.Contains(identifier, "@") { | ||
| 508 | id, ok := st.UserIDByVerifiedEmail(identifier) | ||
| 509 | if !ok { | ||
| 510 | return nil | ||
| 511 | } | ||
| 512 | userID, address = id, identifier | ||
| 513 | } else { | ||
| 514 | u, err := st.UserByName(identifier) | ||
| 515 | if err != nil { | ||
| 516 | return nil | ||
| 517 | } | ||
| 518 | addr, err := st.PrimaryVerifiedEmail(u.ID) | ||
| 519 | if err != nil || addr == "" { | ||
| 520 | return nil | ||
| 521 | } | ||
| 522 | userID, address = u.ID, addr | ||
| 523 | } | ||
| 524 | |||
| 525 | n, err := st.CountLoginTokensSince(userID, time.Now().Add(-time.Hour)) | ||
| 526 | if err != nil { | ||
| 527 | return err | ||
| 528 | } | ||
| 529 | if n >= maxLoginLinksPerHour { | ||
| 530 | return nil | ||
| 531 | } | ||
| 532 | |||
| 533 | token, hash, err := store.NewToken() | ||
| 534 | if err != nil { | ||
| 535 | return err | ||
| 536 | } | ||
| 537 | if err := st.CreateLoginToken(userID, hash, loginLinkTTL); err != nil { | ||
| 538 | return err | ||
| 539 | } | ||
| 540 | host := siteHost(cfg) | ||
| 541 | body := fmt.Sprintf( | ||
| 542 | "Someone (hopefully you) asked to log in to %s.\n\n"+ | ||
| 543 | "Open this link within 15 minutes. It works once:\n\n %s/login?token=%s\n\n"+ | ||
| 544 | "If this wasn't you, ignore this mail. Nothing has changed on the account.\n", | ||
| 545 | host, strings.TrimSuffix(cfg.Server.SiteURL, "/"), token) | ||
| 546 | return mail.Send(cfg, address, "log in to "+host, body) | ||
| 547 | } | ||
| 548 | ``` | ||
| 549 | |||
| 550 | Check `st.UserByName`'s real name and signature before writing this — if the | ||
| 551 | store spells it differently, use the store's spelling rather than adding a | ||
| 552 | wrapper. Run: `grep -n "func (s \*Store) UserByName" internal/store/users.go` | ||
| 553 | |||
| 554 | - [ ] **Step 4: Write the handler** | ||
| 555 | |||
| 556 | In `internal/httpd/accounts.go`, replace `renderLogin` and add `loginSubmit`: | ||
| 557 | |||
| 558 | ```go | ||
| 559 | // renderLogin draws the login page. Mode carries the registration mode so | ||
| 560 | // the page can tell a brand-new visitor how to get an account. EmailLogin | ||
| 561 | // says whether this instance can mail a link; Sent switches the page to the | ||
| 562 | // confirmation that follows a request. | ||
| 563 | func (s *Server) renderLogin(w http.ResponseWriter, errMsg string, sent bool) { | ||
| 564 | s.render(w, "login.html", struct { | ||
| 565 | basePage | ||
| 566 | Mode string // closed | invite | open | ||
| 567 | Error string | ||
| 568 | EmailLogin bool | ||
| 569 | Sent bool | ||
| 570 | }{basePage{Site: s.siteName(), Host: s.cfg.SiteHost()}, | ||
| 571 | s.cfg.Registration.Mode, errMsg, s.emailLoginEnabled(), sent}) | ||
| 572 | } | ||
| 573 | |||
| 574 | // emailLoginEnabled reports whether a link can be mailed at all. There is no | ||
| 575 | // separate switch: the capability is exactly the SMTP the instance already | ||
| 576 | // configured for verification and notification mail. | ||
| 577 | func (s *Server) emailLoginEnabled() bool { | ||
| 578 | return s.cfg.Web.Mode == "accounts" && s.cfg.Mail.SMTPHost != "" | ||
| 579 | } | ||
| 580 | |||
| 581 | // loginSubmit mails a one-time login link. The response is the same page | ||
| 582 | // whatever happened, including when nothing happened. | ||
| 583 | func (s *Server) loginSubmit(w http.ResponseWriter, r *http.Request) { | ||
| 584 | if !s.emailLoginEnabled() { | ||
| 585 | s.notFound(w, r) | ||
| 586 | return | ||
| 587 | } | ||
| 588 | // The per-account bound lives in the store and survives a restart; this | ||
| 589 | // one stops a single source from spending every account's budget. | ||
| 590 | if allowed, wait := s.apiLimit.allow("login"+s.clientIP(r), true); !allowed { | ||
| 591 | w.Header().Set("Retry-After", strconv.Itoa(int(wait.Seconds())+1)) | ||
| 592 | http.Error(w, "too many login requests; wait a moment", http.StatusTooManyRequests) | ||
| 593 | return | ||
| 594 | } | ||
| 595 | if err := control.RequestLoginLink(s.cfg, s.st, r.FormValue("identifier")); err != nil { | ||
| 596 | log.Printf("login link: %v", err) | ||
| 597 | } | ||
| 598 | s.renderLogin(w, "", true) | ||
| 599 | } | ||
| 600 | ``` | ||
| 601 | |||
| 602 | Update the two existing `renderLogin` calls in the `login` handler to pass | ||
| 603 | `false` as the new argument. | ||
| 604 | |||
| 605 | Add `"log"` and `"strconv"` to the file's imports if they are not already | ||
| 606 | there, and `"gitbay.org/gitbay/internal/control"` if absent. | ||
| 607 | |||
| 608 | - [ ] **Step 5: Add the route** | ||
| 609 | |||
| 610 | In `internal/httpd/routes.go`, directly after the `GET /login` line at 100: | ||
| 611 | |||
| 612 | ```go | ||
| 613 | Route{Method: "POST", Pattern: "/login", Mutating: true, | ||
| 614 | Handler: s.checkOrigin(s.loginSubmit)}, | ||
| 615 | ``` | ||
| 616 | |||
| 617 | - [ ] **Step 6: Update the template** | ||
| 618 | |||
| 619 | Replace the top of `internal/web/templates/login.html`, keeping the "New | ||
| 620 | here?" block below it exactly as it is: | ||
| 621 | |||
| 622 | ```html | ||
| 623 | {{define "title"}}login · {{.Site}}{{end}} | ||
| 624 | {{define "content"}} | ||
| 625 | <h1>Log in</h1> | ||
| 626 | {{if .Error}}<p class="error" role="alert">{{.Error}}</p>{{end}} | ||
| 627 | {{if .Sent}} | ||
| 628 | <p>If that account exists, a login link is on its way. It works once and | ||
| 629 | expires in fifteen minutes.</p> | ||
| 630 | {{else}} | ||
| 631 | {{if .EmailLogin}} | ||
| 632 | <form method="post" action="/login"> | ||
| 633 | <label for="identifier">Username or email address</label> | ||
| 634 | <input type="text" id="identifier" name="identifier" autocomplete="username" required> | ||
| 635 | <button type="submit">Email me a link</button> | ||
| 636 | </form> | ||
| 637 | <p>Or, from a machine with your registered key:</p> | ||
| 638 | {{else}} | ||
| 639 | <p>Browser sessions are minted over SSH — there is no password. From a machine | ||
| 640 | with your registered key:</p> | ||
| 641 | {{end}} | ||
| 642 | <pre class="message">ssh git@{{.Host}} web login</pre> | ||
| 643 | <p>then open the printed URL within five minutes.</p> | ||
| 644 | {{end}} | ||
| 645 | ``` | ||
| 646 | |||
| 647 | The `<label for>` is not decoration: `Web:InputWithoutLabelCheck` and #133 | ||
| 648 | cover this, and a placeholder is not an accessible name. | ||
| 649 | |||
| 650 | - [ ] **Step 7: Build, vet, and run the e2e tests** | ||
| 651 | |||
| 652 | ```bash | ||
| 653 | go build ./... && go vet ./... | ||
| 654 | go test ./internal/httpd/ ./internal/control/ ./internal/store/ | ||
| 655 | go test ./e2e/ -run TestEmailLogin -v -timeout 20m | ||
| 656 | ``` | ||
| 657 | Expected: all PASS, including `TestEveryCommandIsReachable` and | ||
| 658 | `TestViewOnlyHasNoMutatingRoutes` in their packages. | ||
| 659 | |||
| 660 | - [ ] **Step 8: Commit** | ||
| 661 | |||
| 662 | ```bash | ||
| 663 | git add internal/control/loginlink.go internal/httpd/accounts.go \ | ||
| 664 | internal/httpd/routes.go internal/web/templates/login.html \ | ||
| 665 | e2e/emaillogin_test.go | ||
| 666 | git commit -m "web: request a login link by email | ||
| 667 | |||
| 668 | An account with no SSH key had no way into the web UI at all: the only | ||
| 669 | caller of CreateWebSession consumed a token that only 'web login' over SSH | ||
| 670 | could mint. An unauthenticated POST /login now mails the same one-time | ||
| 671 | token, throttled per account and per source, with a response that does not | ||
| 672 | vary with whether the account exists. | ||
| 673 | |||
| 674 | Closes #155" | ||
| 675 | ``` | ||
| 676 | |||
| 677 | --- | ||
| 678 | |||
| 679 | ### Task 4: Parity row and the merge request | ||
| 680 | |||
| 681 | The Parity wiki page is a maintained matrix of capability by surface, and the | ||
| 682 | convention is to update the row in the change that moves it. | ||
| 683 | |||
| 684 | **Files:** | ||
| 685 | - Modify: `Parity.org` in the `krz/gitbay.wiki` clone | ||
| 686 | |||
| 687 | - [ ] **Step 1: Clone or update the wiki** | ||
| 688 | |||
| 689 | ```bash | ||
| 690 | cd /tmp && git clone ssh://git@gitbay.org/krz/gitbay.wiki 2>/dev/null || \ | ||
| 691 | (cd /tmp/gitbay.wiki && git pull) | ||
| 692 | ``` | ||
| 693 | |||
| 694 | - [ ] **Step 2: Add the row** | ||
| 695 | |||
| 696 | Open `/tmp/gitbay.wiki/Parity.org`, find the table that carries the | ||
| 697 | authentication and account rows, and add a row for browser login in the same | ||
| 698 | format the neighbouring rows use: available on web, not applicable to CLI or | ||
| 699 | SSH (SSH has `web login`, which is the row above). Match the file's existing | ||
| 700 | markers rather than inventing new ones — read three neighbouring rows first. | ||
| 701 | |||
| 702 | - [ ] **Step 3: Commit and push the wiki** | ||
| 703 | |||
| 704 | ```bash | ||
| 705 | cd /tmp/gitbay.wiki | ||
| 706 | git add Parity.org | ||
| 707 | git commit -m "Parity: browser login by emailed link" | ||
| 708 | git push | ||
| 709 | ``` | ||
| 710 | |||
| 711 | - [ ] **Step 4: Push the branch and open the merge request** | ||
| 712 | |||
| 713 | ```bash | ||
| 714 | cd /Users/cmc/git/krz/gitbay | ||
| 715 | git push -u origin email-login | ||
| 716 | gitbay mr create --source email-login --target main \ | ||
| 717 | --title "Browser login without an SSH key" --file - <<'EOF' | ||
| 718 | An account with no SSH key could not use the web UI at all. `CreateWebSession` | ||
| 719 | has one caller, the `/login?token=` handler, and only `web login` over SSH could | ||
| 720 | mint a token for it. | ||
| 721 | |||
| 722 | An unauthenticated `POST /login` now mails the same one-time token to a verified | ||
| 723 | address, resolved by username or address. Bounded at five an hour per account in | ||
| 724 | the store and by the existing token bucket per source. The response does not | ||
| 725 | vary with whether the account exists, whether its address is verified, or | ||
| 726 | whether it is over its budget. | ||
| 727 | |||
| 728 | The session cookie moves from `SameSite=Strict` to `Lax`, because a link clicked | ||
| 729 | in a mail client is a cross-site navigation and Strict can drop the cookie | ||
| 730 | through the redirect. Every cookie-authenticated mutating route carries | ||
| 731 | `checkOrigin`; the POST routes that do not take only bearer tokens or no auth at | ||
| 732 | all. | ||
| 733 | |||
| 734 | This does not widen what a browser session can do. The web dispatches with | ||
| 735 | `ViaAPI: true`, so no `SSHOnly` command is reachable from one however it was | ||
| 736 | obtained. | ||
| 737 | |||
| 738 | Design: `docs/specs/2026-09-04-email-login-design.md`. | ||
| 739 | Plan: `docs/plans/2026-09-04-email-login.md`. | ||
| 740 | |||
| 741 | Closes #155 | ||
| 742 | EOF | ||
| 743 | ``` | ||
| 744 | |||
| 745 | - [ ] **Step 5: Wait for CI, then merge** | ||
| 746 | |||
| 747 | ```bash | ||
| 748 | gitbay build list --json | ||
| 749 | ``` | ||
| 750 | |||
| 751 | Poll no more often than every 120 seconds: the CLI shares one SSH connection | ||
| 752 | per instance, and the auth limiter reads a burst of failures as an attack. | ||
| 753 | |||
| 754 | When green: | ||
| 755 | |||
| 756 | ```bash | ||
| 757 | gitbay mr merge <n> --strategy squash | ||
| 758 | git checkout main && git pull | ||
| 759 | git branch -d email-login && git push origin --delete email-login | ||
| 760 | ``` | ||
| 761 | |||
| 762 | --- | ||
| 763 | |||
| 764 | ## Self-Review | ||
| 765 | |||
| 766 | **Spec coverage.** Every spec section maps to a task: the exported function, | ||
| 767 | resolution, and mail body to Task 3 Step 3; TTL to `loginLinkTTL`; the durable | ||
| 768 | per-account throttle to Task 1 and its use in Task 3; the per-IP throttle to | ||
| 769 | Task 3 Step 4; enumeration to `TestEmailLoginDoesNotEnumerate`; the empty | ||
| 770 | identifier to the same test; the cookie change to Task 2; the no-new-config | ||
| 771 | decision to `emailLoginEnabled`; out-of-scope signup untouched. The spec's | ||
| 772 | implementation gate on `checkOrigin` coverage was discharged before this plan | ||
| 773 | was written and its result is recorded in Task 2. | ||
| 774 | |||
| 775 | **Deviation.** The return type of `RequestLoginLink` changed from the spec's | ||
| 776 | triple to a single `error`, recorded above under "Change from the spec". | ||
| 777 | |||
| 778 | **Types.** `RequestLoginLink(config.Config, *store.Store, string) error`; | ||
| 779 | `CountLoginTokensSince(int64, time.Time) (int, error)`; | ||
| 780 | `emailLoginEnabled() bool`; `renderLogin(http.ResponseWriter, string, bool)`. | ||
| 781 | Each is used with that signature everywhere it appears. `sessionSameSite` is | ||
| 782 | declared in Task 2 and used in Task 2 only. | ||
| 783 | |||
| 784 | **Unverified at plan time.** `store.UserByName` is used in Task 3 Step 3 but | ||
| 785 | its exact name and signature were not confirmed; Step 3 carries an explicit | ||
| 786 | instruction to check before writing. `Parity.org`'s table format is likewise | ||
| 787 | read at execution rather than guessed. | ||