From 428b42181dbeaa272e1aeaac2a14b39973f0b92b Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 09:44:42 +0200 Subject: [PATCH] mesh-mail: on the operator's Approve the tools record approved and queue the send for the sender, instead of sending it themselves (ADR 0436 clause 8; issue 1194) --- .../cmd/mesh-mail/secret_boundary_test.go | 35 +-- modules/mesh-mail/cmd/mesh-mail/send.go | 157 ++++++++---- modules/mesh-mail/cmd/mesh-mail/send_test.go | 239 +++++++++++++----- modules/mesh-mail/cmd/mesh-mail/sendactor.go | 34 --- modules/mesh-mail/cmd/mesh-mail/tools.go | 24 +- 5 files changed, 306 insertions(+), 183 deletions(-) delete mode 100644 modules/mesh-mail/cmd/mesh-mail/sendactor.go diff --git a/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go b/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go index 539f330b..e819899d 100644 --- a/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go @@ -1,12 +1,12 @@ package main // The tools never open a password file (novox/hq ADR 0436 §12, issue 1193): ReadSecret and SecretPath moved -// into cmd/mesh-mail/internal/secret, imported only by keep (imap.go) and the sender's own actor (sendactor.go, -// which — ahead of issue 1195's separate process — is the one piece of the Serve path still reached from -// Decide's "send" case, exactly as 1194's own report still finds it: "Decide/decideSend call SendActor -// directly"). The build seat runs every test under one account, so no permission test can show the split -// (context file, "Clause 12's proof"); this is the file-level proxy the lead chose instead: an AST check over -// the tools' own files. +// into cmd/mesh-mail/internal/secret, imported only by keep (imap.go) and — once issue 1195 adds it — the +// sender's own process (sender.go). Since issue 1194, Decide no longer reaches any actor that reads a secret +// at all (it records approved and queues the send for the sender); the import list this test checks still +// holds, now even more plainly. The build seat runs every test under one account, so no permission test can +// show the split (context file, "Clause 12's proof"); this is the file-level proxy the lead chose instead: an +// AST check over the tools' own files. import ( "encoding/json" @@ -40,10 +40,11 @@ func TestTheToolsBundleNeverImportsThePasswordPackage(t *testing.T) { } } -// 1193/AC2: keep (imap.go) and the sender's actor (sendactor.go) read the password through the moved package; -// SecretGiven (stat only) stays in config.go, available to the tools, and never reads the password itself. +// 1193/AC2: keep (imap.go) reads the password through the moved package (the sender, issue 1195's sender.go, +// joins this list once it exists); SecretGiven (stat only) stays in config.go, available to the tools, and +// never reads the password itself. func TestKeepAndTheSenderReadThePasswordThroughTheMovedPackage(t *testing.T) { - for _, file := range []string{"imap.go", "sendactor.go"} { + for _, file := range []string{"imap.go"} { fset := token.NewFileSet() f, err := parser.ParseFile(fset, file, nil, parser.ImportsOnly) if err != nil { @@ -76,10 +77,9 @@ func TestKeepAndTheSenderReadThePasswordThroughTheMovedPackage(t *testing.T) { } // 1193/AC3: with the secrets directory absent, mail_send raises its ask, the warrant of an Approve is heard, -// and mail_send_status reads it back — each succeeds, and no error mentions a secret file. (The outcome here -// is "failed: ... has no password yet", because the tools still perform the send through sendactor.go until -// issue 1194 replaces that path with queueing for the sender process; what this proves is narrower and holds -// either way: nothing on this path ever names a secret file or path.) +// and mail_send_status reads it back — each succeeds, and no error mentions a secret file. Since issue 1194 +// the tools never read a secret on this path at all (they queue the send for the sender), so this also proves +// the stronger property the tools' bundle never touches b.Env.Secrets. func TestTheToolPathSucceedsWithTheSecretsDirectoryAbsent(t *testing.T) { dir := t.TempDir() missing := filepath.Join(dir, "never-created") @@ -88,8 +88,8 @@ func TestTheToolPathSucceedsWithTheSecretsDirectoryAbsent(t *testing.T) { "incoming": {"host": "mail.novox.be", "port": 993, "security": "tls"}, "outgoing": {"host": "h", "port": 465, "security": "tls"}}}}`) writeFile(t, env.Grants, `{"grants": {"mesh": ["triage"]}}`) - router, sends := newSendRouter(), newMemSends() - b := &Bundle{Env: env, Now: func() time.Time { return judgedAt }, Asks: router, Sends: sends} + router, sends, queue := newSendRouter(), newMemSends(), newMemQueue() + b := &Bundle{Env: env, Now: func() time.Time { return judgedAt }, Asks: router, Sends: sends, Queue: queue.publish} rt := startBundle(t, b.Tools()) text, isErr := rt.call(SendTool, "novox.triage", sendArgs) @@ -102,10 +102,13 @@ func TestTheToolPathSucceedsWithTheSecretsDirectoryAbsent(t *testing.T) { w := router.chose(id, OptionSend, judgedAt.Add(time.Minute)) router.close(w) - d, err := Decide(sends, b.lookupAccount, b.sendActor(), w, judgedAt.Add(2*time.Minute)) + d, err := Decide(sends, b.queuePublish(), w, judgedAt.Add(2*time.Minute)) if err != nil { t.Fatalf("the warrant was not heard cleanly: %v", err) } + if d.Outcome != "queued" { + t.Fatalf("the decision: %+v; want queued (the tools never read the secrets directory)", d) + } if strings.Contains(d.Outcome, missing) || strings.Contains(strings.ToLower(d.Outcome), "secret") { t.Fatalf("the decision names a secret file: %q", d.Outcome) } diff --git a/modules/mesh-mail/cmd/mesh-mail/send.go b/modules/mesh-mail/cmd/mesh-mail/send.go index bd8f31c6..360213a8 100644 --- a/modules/mesh-mail/cmd/mesh-mail/send.go +++ b/modules/mesh-mail/cmd/mesh-mail/send.go @@ -554,18 +554,32 @@ func looksLikeHostName(w string) bool { // ---- deciding --------------------------------------------------------------------------------------------------- -// AccountLookup resolves an account by name, as the settings currently hold it (not as it stood when it was asked: -// the account's server may have changed since, and its password is read fresh), or found false. -type AccountLookup func(name string) (Account, bool) +// MailSendApproved is the event mesh-mail's tools publish on the work queue mail-sends once the operator's +// Approve is verified and the send recorded approved (novox/hq ADR 0436 clause 8). +const MailSendApproved = "mail.send.approved" -// SendActor performs the one send a chosen option authorises, resolving the account's own secret itself. It is -// called at most once per request (Decide claims the request first). -type SendActor func(a Account, to []string, subject, body string, now time.Time) error +// QueuedSend is the body mail.send.approved carries: never the recipients, the subject or the body, which stay +// in the send's own record in `sends` (novox/hq ADR 0436 clause 8). +type QueuedSend struct { + Send string `json:"send"` + Account string `json:"account"` + Ask string `json:"ask"` +} -// settleSend writes a decision onto a kept send (claiming it, or ending it outright) and, once the write lands, -// drops it from the undecided index: an index entry is never left behind once the record it points to carries a -// decision (novox/hq issue 811). On a race lost to another writer, it reads back whatever that writer decided. -func settleSend(store SendStore, req SendRequest, rev uint64, d SendDecision) (SendDecision, uint64, error) { +// QueuePublish publishes one message on the module's own work-queue stream, the message id chosen (here, the +// send id) so a repeated publish inside the stream's duplicate window is kept to one (novox/hq ADR 0436 +// clauses 6, 8); nothing here reads or holds a password. Real is sdkQueuePublish; a test gives its own. +type QueuePublish func(id string, body any) error + +func sdkQueuePublish(id string, body any) error { + return stdio.Emit(MailSendApproved, body, stdio.EmitOptions{MessageID: id}) +} + +// updateDecision writes a decision onto a kept send by compare-and-set, without touching the undecided index: +// the index is settleSend's to drop, once the decision is one the sweep no longer needs to look at again +// (novox/hq issue 811; ADR 0436 clause 8, where "approved" still needs the sweep). On a race lost to another +// writer, it reads back whatever that writer decided. +func updateDecision(store SendStore, req SendRequest, rev uint64, d SendDecision) (SendDecision, uint64, error) { next := req next.Decided = &d newRev, err := store.Update(req.ID, next, rev) @@ -577,16 +591,30 @@ func settleSend(store SendStore, req SendRequest, rev uint64, d SendDecision) (S } return d, 0, err } - _ = removeUndecided(store, req.ID) // best-effort (see removeUndecided) return d, newRev, nil } +// settleSend writes a decision onto a kept send (claiming it, or ending it outright) and, once the write lands, +// drops it from the undecided index: an index entry is never left behind once the record it points to carries a +// decision the sweep no longer needs to act on (novox/hq issue 811). On a race lost to another writer, it reads +// back whatever that writer decided. +func settleSend(store SendStore, req SendRequest, rev uint64, d SendDecision) (SendDecision, uint64, error) { + final, newRev, err := updateDecision(store, req, rev, d) + if err != nil { + return final, 0, err + } + if newRev != 0 { + _ = removeUndecided(store, req.ID) // best-effort (see removeUndecided) + } + return final, newRev, nil +} + // Decide holds one closed ask's warrant to the request it answers (asks.Warrant.For, asks.Option.Performs), and — -// on Approve, exactly once — performs the send. A request already decided is never decided again: decide answers -// its kept decision at once, and two warrants for the same request (a redelivery, or two readers racing) have only -// the first claim the request by a compare-and-set write; the second finds it already claimed or decided and sends -// nothing. -func Decide(store SendStore, lookup AccountLookup, send SendActor, w asks.Warrant, now time.Time) (SendDecision, error) { +// on Approve, exactly once — records the send approved and queues it for the sender (novox/hq ADR 0436 clause 8); +// it never sends. A request already decided is never decided again: decide answers its kept decision at once, and +// two warrants for the same request (a redelivery, or two readers racing) have only the first claim the request +// by a compare-and-set write; the second finds it already claimed or decided and queues nothing twice. +func Decide(store SendStore, publish QueuePublish, w asks.Warrant, now time.Time) (SendDecision, error) { req, rev, ok, err := getSend(store, w.Ask) if err != nil { return SendDecision{}, err @@ -623,43 +651,45 @@ func Decide(store SendStore, lookup AccountLookup, send SendActor, w asks.Warran case OptionDecline: return settle(SendDecision{At: now, Outcome: "declined", Warrant: w.Says()}) case OptionSend: - return decideSend(store, lookup, send, req, rev, w, now) + return decideSend(store, publish, req, rev, w, now) default: return settle(SendDecision{At: now, Outcome: "refused: the option " + option.ID + " is not one mail_send offers"}) } } -// decideSend claims the request by a compare-and-set write before it sends, so a redelivered warrant finds it -// already claimed (or decided) and acts on nothing: a person's Approve is performed once, never retried on a -// transient failure (the failure is kept in the decision, not hidden). -func decideSend(store SendStore, lookup AccountLookup, send SendActor, req SendRequest, rev uint64, w asks.Warrant, now time.Time) (SendDecision, error) { - claimed := req - claim := SendDecision{At: now, Outcome: "sending", Warrant: w.Says()} - claimedDecision, newRev, err := settleSend(store, req, rev, claim) +// decideSend claims the request as approved by a compare-and-set write before it is queued, so a redelivered +// warrant finds it already claimed (or decided) and queues nothing twice: a person's Approve is queued once +// (novox/hq ADR 0436 clause 8). It never sends: the sender (clause 7) takes the queued message, verifies the +// warrant again itself (clause 9) and performs the one handover to the server (clause 10). +func decideSend(store SendStore, publish QueuePublish, req SendRequest, rev uint64, w asks.Warrant, now time.Time) (SendDecision, error) { + claim := SendDecision{At: now, Outcome: "approved", Warrant: w.Says()} + claimed, newRev, err := updateDecision(store, req, rev, claim) if err != nil { return SendDecision{}, err } if newRev == 0 { - // Lost the claim race: another writer already decided this send. - return claimedDecision, nil + // Lost the claim race: another writer already decided (or is queueing) this send. + return claimed, nil } - claimed.Decided = &claim - acc, found := lookup(req.Account) - var final SendDecision - switch { - case !found: - final = SendDecision{At: now, Outcome: "refused: the account no longer exists", Warrant: w.Says()} - default: - if sendErr := send(acc, req.To, req.Subject, req.Body, now); sendErr != nil { - final = SendDecision{At: now, Outcome: "failed: " + sendErr.Error(), Warrant: w.Says(), Error: sendErr.Error()} - } else { - final = SendDecision{At: now, Outcome: "sent", Warrant: w.Says()} - } + return publishQueued(store, req, newRev, claimed, publish, now) +} + +// publishQueued publishes the approved send on mail-sends, the send id its own message id, and records queued +// once the bus took it. A publish that fails leaves the record approved — never an error returned to the +// caller — so the sweep finds it and publishes it again (novox/hq ADR 0436 clause 8). +func publishQueued(store SendStore, req SendRequest, rev uint64, approved SendDecision, publish QueuePublish, now time.Time) (SendDecision, error) { + if err := publish(req.ID, QueuedSend{Send: req.ID, Account: req.Account, Ask: req.ID}); err != nil { + return approved, nil } - claimed.Decided = &final - if _, err := store.Update(req.ID, claimed, newRev); err != nil { - // The claim stands either way: a person's Approve is acted on once. The caller logs this; it is not retried. - return final, fmt.Errorf("the send %s was %s, and that could not be kept (%w)", req.ID, final.Outcome, err) + queued := SendDecision{At: now, Outcome: "queued", Warrant: approved.Warrant} + final, newRev, err := updateDecision(store, req, rev, queued) + if err != nil { + // The publish stands either way: the bus already took the message once. The caller logs this; it is + // not retried (a retry here would publish a second message under a new id). + return approved, fmt.Errorf("the send %s was queued, and that could not be kept (%w)", req.ID, err) + } + if newRev != 0 { + _ = removeUndecided(store, req.ID) // from here the sender owns it; the tools' sweep is done with it } return final, nil } @@ -670,7 +700,7 @@ func decideSend(store SendStore, lookup AccountLookup, send SendActor, req SendR // declined, ended without a choice (expired, refused, cancelled), or not proven. Read from the router's own // record; when the router's word is a close one that was not decided yet, this is also where a send is first // decided (and so sent), exactly once (Decide). -func ReadSendStatus(store SendStore, bus AskBus, lookup AccountLookup, send SendActor, id string, now time.Time) (map[string]any, error) { +func ReadSendStatus(store SendStore, bus AskBus, publish QueuePublish, id string, now time.Time) (map[string]any, error) { trimmed := strings.TrimSpace(id) req, rev, ok, err := getSend(store, trimmed) if err != nil { @@ -713,7 +743,7 @@ func ReadSendStatus(store SendStore, bus AskBus, lookup AccountLookup, send Send out["status"], out["words"] = "waiting", "the operator has not answered yet" return out, nil } - d, err := Decide(store, lookup, send, *rec.Warrant, now) + d, err := Decide(store, publish, *rec.Warrant, now) if err != nil { out["status"], out["words"] = "not proven", err.Error() return out, nil @@ -729,6 +759,12 @@ func ReadSendStatus(store SendStore, bus AskBus, lookup AccountLookup, send Send func reportDecision(out map[string]any, d SendDecision, now time.Time) { out["at"] = d.At.Format(time.RFC3339) switch { + case d.Outcome == "approved": + // Claimed, not yet queued (the publish to mail-sends is still to do, or failed and the sweep will try it + // again): novox/hq ADR 0436 clause 8. The fuller status enumeration is issue 1199's to settle. + out["status"], out["words"] = "waiting", "approved and waiting for the sender; ask again shortly" + case d.Outcome == "queued": + out["status"], out["words"] = "waiting", "queued for the sender; ask again shortly" case d.Outcome == "sent": out["status"], out["words"] = "sent", "sent on the operator's Approve ("+d.Warrant+")" case d.Outcome == "declined": @@ -758,18 +794,20 @@ func reportDecision(out map[string]any, d SendDecision, now time.Time) { // the same Decide that mail_send_status uses: an approved send goes out as soon as the operator answers, never // only when something later happens to call mail_send_status (novox/hq ADR 0361 §2, review round 1 of issue 790). func (b *Bundle) Heard(w asks.Warrant) { - if _, err := Decide(b.sends(), b.lookupAccount, b.sendActor(), w, b.now()); err != nil { + if _, err := Decide(b.sends(), b.queuePublish(), w, b.now()); err != nil { if b.Logf != nil { b.Logf("[mesh-mail] the warrant for the send %s could not be acted on: %v", w.Ask, err) } } } -// Sweep reads only the sends the undecided index still names, and asks the router's own record for each one's -// answer, the same path mail_send_status takes (ReadSendStatus): an Approve recorded while this module did not -// hear it directly (down, or not yet subscribed) is still acted on, once. Unlike reading every send ever kept, its -// work grows only with the sends still undecided, never with every one a module has ever sent (novox/hq ADR 0361 -// §2, review round 1 of issue 790; the index, issue 811). +// Sweep reads only the sends the undecided index still names. A send not yet decided is asked about the same +// way mail_send_status does (ReadSendStatus): an Approve recorded while this module did not hear it directly +// (down, or not yet subscribed) is still acted on, once. A send left approved (its publish to mail-sends +// failed, or the tools stopped between the two compare-and-set writes) is published again, the send id still +// its message id (novox/hq ADR 0436 clause 8). Unlike reading every send ever kept, its work grows only with +// the sends still undecided, never with every one a module has ever sent (novox/hq ADR 0361 §2, review round 1 +// of issue 790; the index, issue 811). func (b *Bundle) Sweep(now time.Time) { store := b.sends() ids, err := undecidedIDs(store) @@ -780,13 +818,22 @@ func (b *Bundle) Sweep(now time.Time) { return } for _, id := range ids { - req, _, ok, err := getSend(store, id) - if err != nil || !ok || req.Decided != nil { + req, rev, ok, err := getSend(store, id) + if err != nil || !ok { continue } - if _, err := ReadSendStatus(store, b.askBus(), b.lookupAccount, b.sendActor(), id, now); err != nil { - if b.Logf != nil { - b.Logf("[mesh-mail] sweeping the send %s: %v", id, err) + switch { + case req.Decided == nil: + if _, err := ReadSendStatus(store, b.askBus(), b.queuePublish(), id, now); err != nil { + if b.Logf != nil { + b.Logf("[mesh-mail] sweeping the send %s: %v", id, err) + } + } + case req.Decided.Outcome == "approved": + if _, err := publishQueued(store, req, rev, *req.Decided, b.queuePublish(), now); err != nil { + if b.Logf != nil { + b.Logf("[mesh-mail] sweeping the approved send %s: %v", id, err) + } } } } diff --git a/modules/mesh-mail/cmd/mesh-mail/send_test.go b/modules/mesh-mail/cmd/mesh-mail/send_test.go index 176a7287..59e65438 100644 --- a/modules/mesh-mail/cmd/mesh-mail/send_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/send_test.go @@ -166,6 +166,47 @@ func (r *sendRouter) published1() int { return len(r.published) } +// ---- a fake work-queue publish, as the tools reach mail-sends (novox/hq ADR 0436 clause 8) ---------------------- + +type queuedMsg struct { + id string + body QueuedSend +} + +type memQueue struct { + mu sync.Mutex + published []queuedMsg + failErr error +} + +func newMemQueue() *memQueue { return &memQueue{} } + +func (q *memQueue) publish(id string, body any) error { + q.mu.Lock() + defer q.mu.Unlock() + if q.failErr != nil { + return q.failErr + } + b, _ := body.(QueuedSend) + q.published = append(q.published, queuedMsg{id: id, body: b}) + return nil +} + +func (q *memQueue) count() int { + q.mu.Lock() + defer q.mu.Unlock() + return len(q.published) +} + +func (q *memQueue) last() queuedMsg { + q.mu.Lock() + defer q.mu.Unlock() + if len(q.published) == 0 { + return queuedMsg{} + } + return q.published[len(q.published)-1] +} + // ---- an in-process fake SMTP server, speaking the protocol over a real socket ------------------------------------- type receivedMail struct { @@ -375,11 +416,13 @@ func TestMailSendAsksWithTheAccountRecipientsSubjectAndFirstLines(t *testing.T) } } -// (3) On Approve, the fake SMTP server receives exactly one message, even if the warrant is delivered twice. -func TestOnApproveTheFakeSMTPServerReceivesExactlyOneMessageEvenDeliveredTwice(t *testing.T) { - fake := newFakeSMTPServer(t) +// 1194/AC1: On Approve, the send is queued for the sender exactly once, even if the warrant is delivered twice +// (novox/hq ADR 0436 clause 8). Since issue 1194 the tools never dial an outgoing server: they record approved +// and queue the send for the sender process, which alone performs it (issue 1196, 1198). +func TestOnApproveTheSendIsQueuedExactlyOnceEvenDeliveredTwice(t *testing.T) { b, router, sends := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) - b.Dial = fake.dialer() + queue := newMemQueue() + b.Queue = queue.publish rt := startBundle(t, b.Tools()) text, isErr := rt.call(SendTool, "novox.triage", sendArgs) @@ -393,44 +436,41 @@ func TestOnApproveTheFakeSMTPServerReceivesExactlyOneMessageEvenDeliveredTwice(t w := router.chose(id, OptionSend, judgedAt.Add(time.Minute)) router.close(w) - lookup := b.lookupAccount - actor := b.sendActor() - if _, err := Decide(sends, lookup, actor, w, judgedAt.Add(2*time.Minute)); err != nil { + publish := b.queuePublish() + if _, err := Decide(sends, publish, w, judgedAt.Add(2*time.Minute)); err != nil { t.Fatalf("deciding: %v", err) } - if n := fake.count(); n != 1 { - t.Fatalf("the fake server received %d message(s) after one Approve", n) + if n := queue.count(); n != 1 { + t.Fatalf("the queue received %d message(s) after one Approve", n) } - // The warrant delivered a second time: still exactly one message. - if _, err := Decide(sends, lookup, actor, w, judgedAt.Add(3*time.Minute)); err != nil { + // The warrant delivered a second time: still queued exactly once. + if _, err := Decide(sends, publish, w, judgedAt.Add(3*time.Minute)); err != nil { t.Fatalf("deciding again: %v", err) } - if n := fake.count(); n != 1 { - t.Fatalf("the fake server received %d message(s) after the warrant was delivered twice", n) + if n := queue.count(); n != 1 { + t.Fatalf("the queue received %d message(s) after the warrant was delivered twice", n) } - got := fake.last() - if got.from != "jschoubben@novox.be" || len(got.to) != 2 || got.to[0] != "ops@example.org" || got.to[1] != "lead@example.org" { - t.Fatalf("received %+v", got) - } - if !strings.Contains(got.data, "The nightly gate is red") || !strings.Contains(got.data, "The merge gate failed on the resolver change") { - t.Fatalf("the message sent: %q", got.data) + got := queue.last() + if got.id != id || got.body.Send != id || got.body.Account != "mesh" || got.body.Ask != id { + t.Fatalf("queued %+v, want the send id %s as the message id, account and ask", got, id) } - // mail_send_status reports it sent, and reports the same thing again without sending a third time. + // mail_send_status reports it waiting on the sender, and reports the same thing again without queueing a + // third time. text, isErr = rt.call(SendStatusTool, "novox.triage", map[string]any{"id": id}) - if isErr || !strings.Contains(text, `"status":"sent"`) { + if isErr || !strings.Contains(text, `"status":"waiting"`) { t.Fatalf("%v %s", isErr, text) } - if n := fake.count(); n != 1 { - t.Fatalf("mail_send_status sent again: %d", n) + if n := queue.count(); n != 1 { + t.Fatalf("mail_send_status queued again: %d", n) } } -// (4) On Decline, nothing is sent, and the status says declined. -func TestOnDeclineNothingIsSentAndTheStatusSaysDeclined(t *testing.T) { - fake := newFakeSMTPServer(t) +// 1194/AC3: On Decline, nothing is queued, and the status says declined. +func TestOnDeclineNothingIsQueuedAndTheStatusSaysDeclined(t *testing.T) { b, router, sends := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) - b.Dial = fake.dialer() + queue := newMemQueue() + b.Queue = queue.publish rt := startBundle(t, b.Tools()) text, isErr := rt.call(SendTool, "novox.triage", sendArgs) @@ -443,11 +483,11 @@ func TestOnDeclineNothingIsSentAndTheStatusSaysDeclined(t *testing.T) { w := router.chose(id, OptionDecline, judgedAt.Add(time.Minute)) router.close(w) - if _, err := Decide(sends, b.lookupAccount, b.sendActor(), w, judgedAt.Add(2*time.Minute)); err != nil { + if _, err := Decide(sends, b.queuePublish(), w, judgedAt.Add(2*time.Minute)); err != nil { t.Fatalf("deciding: %v", err) } - if n := fake.count(); n != 0 { - t.Fatalf("the fake server received %d message(s) on Decline", n) + if n := queue.count(); n != 0 { + t.Fatalf("the queue received %d message(s) on Decline", n) } text, isErr = rt.call(SendStatusTool, "novox.triage", map[string]any{"id": id}) @@ -474,12 +514,12 @@ func TestMailSendRefusesAnAccountWithNoOutgoingServer(t *testing.T) { } } -// (5) An approved send goes out once the warrant reaches the module through the hear handler alone — never -// through mail_send_status (novox/hq ADR 0361 §2, review round 1 of issue 790). -func TestAnApprovedSendGoesOutOnceWithNoStatusCall(t *testing.T) { - fake := newFakeSMTPServer(t) +// 1194/AC1: An approved send is queued once the warrant reaches the module through the hear handler alone — +// never through mail_send_status (novox/hq ADR 0361 §2, review round 1 of issue 790). +func TestAnApprovedSendIsQueuedOnceWithNoStatusCall(t *testing.T) { b, router, _ := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) - b.Dial = fake.dialer() + queue := newMemQueue() + b.Queue = queue.publish rt := startBundle(t, b.Tools()) text, isErr := rt.call(SendTool, "novox.triage", sendArgs) @@ -494,21 +534,21 @@ func TestAnApprovedSendGoesOutOnceWithNoStatusCall(t *testing.T) { router.close(w) b.Heard(w) // the warrant delivered through the hear handler only - if n := fake.count(); n != 1 { - t.Fatalf("the fake server received %d message(s) after the warrant was heard once", n) + if n := queue.count(); n != 1 { + t.Fatalf("the queue received %d message(s) after the warrant was heard once", n) } b.Heard(w) // the same warrant delivered a second time - if n := fake.count(); n != 1 { - t.Fatalf("the fake server received %d message(s) after the warrant was heard twice", n) + if n := queue.count(); n != 1 { + t.Fatalf("the queue received %d message(s) after the warrant was heard twice", n) } } -// (6) The sweep acts on a send the operator approved while this module was never told directly: it reads the -// router's own record, the same path mail_send_status takes (novox/hq issue 790, review round 1). +// 1194/AC1: The sweep acts on a send the operator approved while this module was never told directly: it reads +// the router's own record, the same path mail_send_status takes (novox/hq issue 790, review round 1). func TestSweepActsOnAnApprovedSendItWasNotToldAboutDirectly(t *testing.T) { - fake := newFakeSMTPServer(t) b, router, _ := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) - b.Dial = fake.dialer() + queue := newMemQueue() + b.Queue = queue.publish rt := startBundle(t, b.Tools()) text, isErr := rt.call(SendTool, "novox.triage", sendArgs) @@ -523,12 +563,60 @@ func TestSweepActsOnAnApprovedSendItWasNotToldAboutDirectly(t *testing.T) { router.close(w) // only the router's record holds the answer; mesh-mail is never told directly b.Sweep(judgedAt.Add(2 * time.Minute)) - if n := fake.count(); n != 1 { + if n := queue.count(); n != 1 { t.Fatalf("the sweep did not act on the approved send: %d message(s)", n) } b.Sweep(judgedAt.Add(3 * time.Minute)) - if n := fake.count(); n != 1 { - t.Fatalf("a second sweep sent again: %d message(s)", n) + if n := queue.count(); n != 1 { + t.Fatalf("a second sweep queued again: %d message(s)", n) + } +} + +// 1194/AC2: A send left "approved" (the publish to mail-sends failed, or the tools stopped between the two +// compare-and-set writes) is still found by the undecided index, and the sweep publishes it again, still under +// the send's own id (novox/hq ADR 0436 clause 8). +func TestSweepPublishesAgainASendLeftApproved(t *testing.T) { + b, router, sends := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) + queue := newMemQueue() + queue.failErr = errors.New("the bus is down") + b.Queue = queue.publish + rt := startBundle(t, b.Tools()) + + text, isErr := rt.call(SendTool, "novox.triage", sendArgs) + if isErr { + t.Fatalf("%s", text) + } + var out map[string]any + _ = json.Unmarshal([]byte(text), &out) + id := out["ask"].(string) + + w := router.chose(id, OptionSend, judgedAt.Add(time.Minute)) + router.close(w) + if _, err := Decide(sends, b.queuePublish(), w, judgedAt.Add(2*time.Minute)); err != nil { + t.Fatalf("deciding: %v", err) + } + if n := queue.count(); n != 0 { + t.Fatalf("the queue received %d message(s) despite failing", n) + } + req, _, ok, err := getSend(sends, id) + if err != nil || !ok || req.Decided == nil || req.Decided.Outcome != "approved" { + t.Fatalf("left approved: %v %v %+v", err, ok, req.Decided) + } + + // Once the bus comes back, the sweep publishes the approved send again, and it reaches queued. + queue.failErr = nil + b.Sweep(judgedAt.Add(3 * time.Minute)) + if n := queue.count(); n != 1 { + t.Fatalf("the sweep did not publish the approved send: %d message(s)", n) + } + req, _, ok, err = getSend(sends, id) + if err != nil || !ok || req.Decided == nil || req.Decided.Outcome != "queued" { + t.Fatalf("not queued after the sweep: %v %v %+v", err, ok, req.Decided) + } + // A further sweep does not publish a send that already reached queued (it left the undecided index). + b.Sweep(judgedAt.Add(4 * time.Minute)) + if n := queue.count(); n != 1 { + t.Fatalf("a further sweep published again: %d message(s)", n) } } @@ -590,26 +678,15 @@ func TestMailSendRefusesAControlCharacterInTheSubjectOrAnAddress(t *testing.T) { } // (9) Rcpt takes the parsed address alone; the To header takes the formatted address (novox/hq issue 790, review -// round 1). +// round 1). Tested directly against SendMail (smtp.go): since issue 1194 the tools never dial an outgoing +// server themselves — Decide only records approved and queues the send for the sender (issue 1196, 1198), which +// is where SendMail is actually called from. func TestSendUsesTheParsedAddressForRcptAndTheFormattedAddressForTheToHeader(t *testing.T) { fake := newFakeSMTPServer(t) - b, router, _ := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) - b.Dial = fake.dialer() - rt := startBundle(t, b.Tools()) - - args := map[string]any{"account": "mesh", "to": []any{"Ops Team "}, - "subject": "Status", "body": "body"} - text, isErr := rt.call(SendTool, "novox.triage", args) - if isErr { - t.Fatalf("%s", text) + acc := Account{Name: "mesh", Address: "jschoubben@novox.be", Outgoing: &Server{Host: "127.0.0.1", Port: 25, Security: "tls"}} + if err := SendMail(fake.dialer(), acc, "the-password", []string{"Ops Team "}, "Status", "body", judgedAt); err != nil { + t.Fatalf("SendMail: %v", err) } - var out map[string]any - _ = json.Unmarshal([]byte(text), &out) - id := out["ask"].(string) - - w := router.chose(id, OptionSend, judgedAt.Add(time.Minute)) - router.close(w) - b.Heard(w) got := fake.last() if len(got.to) != 1 || got.to[0] != "ops@example.org" { @@ -671,9 +748,7 @@ func TestASendWhoseAskNeverPublishedEndsAfterTheAskWouldHaveLasted(t *testing.T) // (11) The sweep reads only the sends still undecided, by the undecided index, not every send ever kept: a send // already decided is never asked about again (novox/hq issue 811). func TestSweepReadsOnlyUndecidedSends(t *testing.T) { - fake := newFakeSMTPServer(t) b, router, sends := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) - b.Dial = fake.dialer() rt := startBundle(t, b.Tools()) // One send, declined right away: decided, so it leaves the undecided index. @@ -686,7 +761,7 @@ func TestSweepReadsOnlyUndecidedSends(t *testing.T) { declinedID := out["ask"].(string) w := router.chose(declinedID, OptionDecline, judgedAt.Add(time.Minute)) router.close(w) - if _, err := Decide(sends, b.lookupAccount, b.sendActor(), w, judgedAt.Add(2*time.Minute)); err != nil { + if _, err := Decide(sends, b.queuePublish(), w, judgedAt.Add(2*time.Minute)); err != nil { t.Fatalf("deciding: %v", err) } @@ -748,3 +823,35 @@ func TestAddAndRemoveUndecidedFailOnACorruptIndexRatherThanResettingIt(t *testin t.Fatalf("removeUndecided overwrote the corrupt index instead of leaving it: %s", sends.vals[undecidedIndexKey]) } } + +// 1194/AC3: a warrant that does not bind the request (its digest is not the ask's) is refused, as today, and +// nothing is published on mail-sends. +func TestADigestMismatchIsRefusedAndNothingIsQueued(t *testing.T) { + b, router, sends := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) + queue := newMemQueue() + b.Queue = queue.publish + rt := startBundle(t, b.Tools()) + + text, isErr := rt.call(SendTool, "novox.triage", sendArgs) + if isErr { + t.Fatalf("%s", text) + } + var out map[string]any + _ = json.Unmarshal([]byte(text), &out) + id := out["ask"].(string) + + w := router.chose(id, OptionSend, judgedAt.Add(time.Minute)) + w.AskDigest = "sha256:" + strings.Repeat("0", 64) // not the digest of the ask actually published + router.close(w) + + d, err := Decide(sends, b.queuePublish(), w, judgedAt.Add(2*time.Minute)) + if err != nil { + t.Fatalf("deciding: %v", err) + } + if !strings.HasPrefix(d.Outcome, "refused: ") { + t.Fatalf("outcome: %q, want refused: ...", d.Outcome) + } + if n := queue.count(); n != 0 { + t.Fatalf("the queue received %d message(s) despite the digest mismatch", n) + } +} diff --git a/modules/mesh-mail/cmd/mesh-mail/sendactor.go b/modules/mesh-mail/cmd/mesh-mail/sendactor.go deleted file mode 100644 index 247b5d3c..00000000 --- a/modules/mesh-mail/cmd/mesh-mail/sendactor.go +++ /dev/null @@ -1,34 +0,0 @@ -package main - -// The one piece of the Serve path that still reads a password (novox/hq ADR 0436 §12, issue 1193): moved out -// of tools.go so a test (secret_boundary_test.go) can prove the tools' own files — main.go's serve path, -// tools.go, send.go, health.go — never import the password-reading package. Decide still calls this directly -// (send.go's Heard and Sweep, tools.go's mail_send_status): issue 1194 replaces that with the tools queueing -// the approved send for the sender process instead, at which point this file's job moves to it (issue 1195) -// and this one is removed. - -import ( - "fmt" - "time" - - "mesh-mail/cmd/mesh-mail/internal/secret" -) - -// sendActor performs one send on the operator's Approve: it reads the account's own secret itself, so Decide and -// the `sends` state never hold a password. -func (b *Bundle) sendActor() SendActor { - dial := b.Dial - if dial == nil { - dial = DialSMTP - } - return func(a Account, to []string, subject, body string, now time.Time) error { - pw, given, err := secret.Read(b.Env.Secrets, a.Secret()) - if err != nil { - return fmt.Errorf("the account %s's password cannot be read: %w", a.Name, err) - } - if !given { - return fmt.Errorf("the account %s has no password yet; the operator gives it at the desk", a.Name) - } - return SendMail(dial, a, pw, to, subject, body, now) - } -} diff --git a/modules/mesh-mail/cmd/mesh-mail/tools.go b/modules/mesh-mail/cmd/mesh-mail/tools.go index fb5e3fe7..17a0d7f9 100644 --- a/modules/mesh-mail/cmd/mesh-mail/tools.go +++ b/modules/mesh-mail/cmd/mesh-mail/tools.go @@ -45,9 +45,10 @@ const ( ) // Bundle is what the tools reach: the settings, the index, the attachment store, and — for mail_send — the -// operator channel, the `sends` state and the outgoing dialer. The last three are nil outside a test: the real -// bus and dialer (sdkAskBus, keptSends, DialSMTP) are used lazily, so building a Bundle for the read-only tools -// never needs them. +// operator channel, the `sends` state and the work-queue publisher. The last three are nil outside a test: the +// real bus (sdkAskBus, keptSends, sdkQueuePublish) is used lazily, so building a Bundle for the read-only tools +// never needs them. Nothing here reads a password: the tools record the operator's Approve and queue the send +// for the sender, which alone performs it (novox/hq ADR 0436 §12, issue 1193, 1194). type Bundle struct { Env Env Index func(ctx context.Context) (Store, error) @@ -57,7 +58,7 @@ type Bundle struct { Asks AskBus Sends SendStore - Dial SMTPDialer + Queue QueuePublish Logf func(format string, a ...any) } @@ -397,14 +398,13 @@ func (b *Bundle) sends() SendStore { return keptSends{stdio.State(SendsState)} } -// lookupAccount resolves an account by name as the settings currently hold it — read fresh at decide time, never -// the account as it stood when it was asked. -func (b *Bundle) lookupAccount(name string) (Account, bool) { - accounts, err := ReadAccounts(b.Env.Accounts) - if err != nil { - return Account{}, false +// queuePublish is how this bundle publishes onto the module's own work-queue stream: the bus's own, unless a +// test gave one. Nothing here reads a password (novox/hq ADR 0436 §12). +func (b *Bundle) queuePublish() QueuePublish { + if b.Queue != nil { + return b.Queue } - return accountByName(accounts, name) + return sdkQueuePublish } func accountByName(accounts []Account, name string) (Account, bool) { @@ -464,7 +464,7 @@ func (b *Bundle) sendStatusTool(args map[string]any) (any, error) { if id == "" { return nil, fmt.Errorf("%s needs id, the ask %s gave", SendStatusTool, SendTool) } - return ReadSendStatus(b.sends(), b.askBus(), b.lookupAccount, b.sendActor(), id, b.now()) + return ReadSendStatus(b.sends(), b.askBus(), b.queuePublish(), id, b.now()) } // ---- arguments ----------------------------------------------------------------------------------------------------