diff --git a/internal/core/recovery/flow.go b/internal/core/recovery/flow.go index 94eab27..2601610 100644 --- a/internal/core/recovery/flow.go +++ b/internal/core/recovery/flow.go @@ -72,6 +72,9 @@ type Deps struct { BaseURL string // PendingTTL bounds the approvable window (default DefaultPendingTTL). PendingTTL time.Duration + // Dispatch runs a notification OFF the request path (M2s). Defaults to + // `go fn()`; tests inject a synchronous runner for determinism. + Dispatch func(fn func()) // Logger receives structured logs. Defaults to slog.Default(). Logger *slog.Logger // Now is the clock (tests). Defaults to time.Now. @@ -104,6 +107,9 @@ func New(d Deps) (*Actions, error) { if d.Now == nil { d.Now = time.Now } + if d.Dispatch == nil { + d.Dispatch = func(fn func()) { go fn() } + } if d.PendingTTL <= 0 { d.PendingTTL = DefaultPendingTTL } @@ -155,10 +161,36 @@ func (a *Actions) openRequest(ctx context.Context, requester users.User) { return } if created { - a.notify(ctx, requester, req) + a.notifyAsync(ctx, requester, req) } } +// notifyTimeout bounds a detached notification's work. +const notifyTimeout = 30 * time.Second + +// notifyAsync delivers the notifications OFF the request path (M2s): an /access +// HIT must not take measurably longer than a MISS, or the timing difference is +// an enumeration oracle (A3). The work is detached from the request context +// (which is cancelled as soon as the response is written) but keeps its values, +// so the audit trail is intact. +// +// A panic here is NOT recovered by net/http (it is no longer a request +// goroutine), so it is recovered here: a notification failure must never take +// the process down. +func (a *Actions) notifyAsync(ctx context.Context, requester users.User, req Request) { + base := context.WithoutCancel(ctx) + a.d.Dispatch(func() { + defer func() { + if p := recover(); p != nil { + a.d.Logger.Error("recovery: notification panicked", "panic", p) + } + }() + nctx, cancel := context.WithTimeout(base, notifyTimeout) + defer cancel() + a.notify(nctx, requester, req) + }) +} + // notify emails the approver (the requester's manager, the chain link above // them) and the employee (SPEC-007 §4: "notify the approver chain + employee"). func (a *Actions) notify(ctx context.Context, requester users.User, req Request) { diff --git a/internal/core/recovery/flow_test.go b/internal/core/recovery/flow_test.go new file mode 100644 index 0000000..31b1090 --- /dev/null +++ b/internal/core/recovery/flow_test.go @@ -0,0 +1,122 @@ +package recovery_test + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "net/url" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/audit" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/auth" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/httpx" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/mail" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/recovery" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/store" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/users" +) + +// blockingMailer blocks every Send until release is closed, and signals when a +// send has begun. +type blockingMailer struct { + started chan struct{} + release chan struct{} + done chan struct{} + once sync.Once +} + +func (m *blockingMailer) Send(_ context.Context, _ mail.Message) error { + select { + case m.started <- struct{}{}: + default: + } + <-m.release + m.once.Do(func() { close(m.done) }) + return nil +} + +// TestAccessNotificationIsAsynchronous covers M2s: POST /access must answer +// uniformly whether or not the email matched, so a HIT's notification is +// delivered OFF the request path. A synchronous implementation would block the +// response on the (slow) mailer and leak "is this email on record?". +func TestAccessNotificationIsAsynchronous(t *testing.T) { + ctx := context.Background() + + db, err := store.Open(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("store.Open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + if err := db.Migrate(); err != nil { + t.Fatalf("Migrate: %v", err) + } + + aud := audit.New(db.DB) + us := users.New(db.DB, aud) + if _, err := us.Create(ctx, 0, "admin@example.com", "A", users.RoleAdmin); err != nil { + t.Fatalf("create user: %v", err) + } + + sealer, err := auth.NewAEADSealer(bytes.Repeat([]byte{0x7a}, auth.KeySize)) + if err != nil { + t.Fatalf("NewAEADSealer: %v", err) + } + core := auth.New(db.DB, aud, auth.WithSealer(sealer)) + pre, err := httpx.NewPreSessionCSRF([]byte("flow-cookie-secret-0123456789abcdef"), false) + if err != nil { + t.Fatalf("NewPreSessionCSRF: %v", err) + } + + m := &blockingMailer{ + started: make(chan struct{}, 1), + release: make(chan struct{}), + done: make(chan struct{}), + } + // Dispatch is deliberately NOT injected: the default must be asynchronous. + a, err := recovery.New(recovery.Deps{ + Requests: recovery.NewStore(db.DB, aud), + Users: us, + Identities: core.Identities(), + Mailer: m, + CSRF: pre, + Audit: aud, + BaseURL: "https://myday.example.com", + }) + if err != nil { + t.Fatalf("recovery.New: %v", err) + } + + form := url.Values{"email": {"admin@example.com"}}.Encode() + req := httptest.NewRequest("POST", "/access?do=auth&action=recover", strings.NewReader(form)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + + returned := make(chan error, 1) + go func() { returned <- a.Request(rec, req) }() + + select { + case err := <-returned: + if err != nil { + t.Fatalf("Request: %v", err) + } + case <-time.After(2 * time.Second): + close(m.release) + t.Fatal("POST /access blocked on the notification — it is still synchronous (M2s)") + } + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want 200", rec.Code) + } + + // The notification did run (off the request path); let it finish. + close(m.release) + select { + case <-m.done: + case <-time.After(2 * time.Second): + t.Fatal("the detached notification never ran") + } +}