From 03c3f56a5938e34fea3785da9a8e9c578c699cff Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien <mechiel@ueber.net> Date: Sun, 2 Jul 2023 13:53:34 +0200 Subject: [PATCH] add basic tests for the ctl subcommands, and fix two small bugs this doesn't really test the output of the ctl commands, just that they succeed without error. better than nothing... testing found two small bugs, that are not an issue in practice: 1. we were ack'ing streamed data from the other side of the ctl connection before having read it. when there is no buffer space on the connection (always the case for net.Pipe) that would cause a deadlock. only actually happened during the new tests. 2. the generated dkim keys are relatively to the directory of the dynamic config file. mox looked it up relative to the directory of the _static_ config file at startup. this directory is typicaly the same. users would have noticed if they had triggered this. --- .gitignore | 2 + Makefile | 2 +- ctl.go | 11 ++- ctl_test.go | 180 ++++++++++++++++++++++++++++++++++++++ import.go | 61 ++++--------- integration_test.go | 7 +- main.go | 138 ++++++++++++++++++----------- mox-/admin.go | 2 +- quickstart_test.go | 9 +- testdata/ctl/domains.conf | 7 ++ testdata/ctl/mox.conf | 9 ++ 11 files changed, 321 insertions(+), 107 deletions(-) create mode 100644 ctl_test.go create mode 100644 testdata/ctl/domains.conf create mode 100644 testdata/ctl/mox.conf diff --git a/.gitignore b/.gitignore index 85eaa82..232c1cd 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,8 @@ /rfc/[0-9][0-9]* /local/ /testdata/check/ +/testdata/ctl/data/ +/testdata/ctl/dkim/ /testdata/empty/ /testdata/exportmaildir/ /testdata/exportmbox/ diff --git a/Makefile b/Makefile index 88f7090..541093d 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ build: # build early to catch syntax errors CGO_ENABLED=0 go build CGO_ENABLED=0 go vet -tags integration - CGO_ENABLED=0 go vet -tags quickstart quickstart_test.go + CGO_ENABLED=0 go vet -tags quickstart ./gendoc.sh (cd http && CGO_ENABLED=0 go run ../vendor/github.com/mjl-/sherpadoc/cmd/sherpadoc/*.go -adjust-function-names none Admin) >http/adminapi.json (cd http && CGO_ENABLED=0 go run ../vendor/github.com/mjl-/sherpadoc/cmd/sherpadoc/*.go -adjust-function-names none Account) >http/accountapi.json diff --git a/ctl.go b/ctl.go index 20cd627..f8d50c8 100644 --- a/ctl.go +++ b/ctl.go @@ -232,8 +232,6 @@ func (s *ctlreader) Read(buf []byte) (N int, Err error) { return 0, s.err } s.npending = int(n) - _, err = fmt.Fprintln(s.conn, "ok") - s.xcheck(err, "writing ok after reading") } rn := len(buf) if rn > s.npending { @@ -242,6 +240,10 @@ func (s *ctlreader) Read(buf []byte) (N int, Err error) { n, err := s.r.Read(buf[:rn]) s.xcheck(err, "read from ctl") s.npending -= n + if s.npending == 0 { + _, err = fmt.Fprintln(s.conn, "ok") + s.xcheck(err, "writing ok after reading") + } return n, err } @@ -286,11 +288,12 @@ func servectl(ctx context.Context, log *mlog.Log, conn net.Conn, shutdown func() ctl.xwrite("ctlv0") for { - servectlcmd(ctx, log, ctl, shutdown) + servectlcmd(ctx, ctl, shutdown) } } -func servectlcmd(ctx context.Context, log *mlog.Log, ctl *ctl, shutdown func()) { +func servectlcmd(ctx context.Context, ctl *ctl, shutdown func()) { + log := ctl.log cmd := ctl.xread() ctl.cmd = cmd log.Info("ctl command", mlog.Field("cmd", cmd)) diff --git a/ctl_test.go b/ctl_test.go new file mode 100644 index 0000000..a9efe96 --- /dev/null +++ b/ctl_test.go @@ -0,0 +1,180 @@ +//go:build !quickstart && !integration + +package main + +import ( + "context" + "flag" + "net" + "os" + "testing" + + "github.com/mjl-/mox/dmarcdb" + "github.com/mjl-/mox/dns" + "github.com/mjl-/mox/mlog" + "github.com/mjl-/mox/mox-" + "github.com/mjl-/mox/mtastsdb" + "github.com/mjl-/mox/queue" + "github.com/mjl-/mox/store" + "github.com/mjl-/mox/tlsrptdb" +) + +var ctxbg = context.Background() + +func tcheck(t *testing.T, err error, errmsg string) { + if err != nil { + t.Helper() + t.Fatalf("%s: %v", errmsg, err) + } +} + +// TestCtl executes commands through ctl. This tests at least the protocols (who +// sends when/what) is tested. We often don't check the actual results, but +// unhandled errors would cause a panic. +func TestCtl(t *testing.T) { + os.RemoveAll("testdata/ctl/data") + mox.ConfigStaticPath = "testdata/ctl/mox.conf" + mox.ConfigDynamicPath = "testdata/ctl/domains.conf" + if errs := mox.LoadConfig(ctxbg, true, false); len(errs) > 0 { + t.Fatalf("loading mox config: %v", errs) + } + switchDone := store.Switchboard() + defer close(switchDone) + + xlog := mlog.New("ctl") + + testctl := func(fn func(clientctl *ctl)) { + t.Helper() + + cconn, sconn := net.Pipe() + clientctl := ctl{conn: cconn, log: xlog} + serverctl := ctl{conn: sconn, log: xlog} + go servectlcmd(ctxbg, &serverctl, func() {}) + fn(&clientctl) + cconn.Close() + sconn.Close() + } + + // "deliver" + testctl(func(ctl *ctl) { + ctlcmdDeliver(ctl, "mjl@mox.example") + }) + + // "setaccountpassword" + testctl(func(ctl *ctl) { + ctlcmdSetaccountpassword(ctl, "mjl@mox.example", "test4321") + }) + + err := queue.Init() + tcheck(t, err, "queue init") + + // "queue" + testctl(func(ctl *ctl) { + ctlcmdQueueList(ctl) + }) + + // "queuekick" + testctl(func(ctl *ctl) { + ctlcmdQueueKick(ctl, 0, "", "", "") + }) + + // "queuedrop" + testctl(func(ctl *ctl) { + ctlcmdQueueDrop(ctl, 0, "", "") + }) + + // no "queuedump", we don't have a message to dump, and the commands exits without a message. + + // "importmbox" + testctl(func(ctl *ctl) { + ctlcmdImport(ctl, true, "mjl", "inbox", "testdata/importtest.mbox") + }) + + // "importmaildir" + testctl(func(ctl *ctl) { + ctlcmdImport(ctl, false, "mjl", "inbox", "testdata/importtest.maildir") + }) + + // "domainadd" + testctl(func(ctl *ctl) { + ctlcmdConfigDomainAdd(ctl, dns.Domain{ASCII: "mox2.example"}, "mjl", "") + }) + + // "accountadd" + testctl(func(ctl *ctl) { + ctlcmdConfigAccountAdd(ctl, "mjl2", "mjl2@mox2.example") + }) + + // "addressadd" + testctl(func(ctl *ctl) { + ctlcmdConfigAddressAdd(ctl, "mjl3@mox2.example", "mjl2") + }) + + // Add a message. + testctl(func(ctl *ctl) { + ctlcmdDeliver(ctl, "mjl3@mox2.example") + }) + // "retrain", retrain junk filter. + testctl(func(ctl *ctl) { + ctlcmdRetrain(ctl, "mjl2") + }) + + // "addressrm" + testctl(func(ctl *ctl) { + ctlcmdConfigAddressRemove(ctl, "mjl3@mox2.example") + }) + + // "accountrm" + testctl(func(ctl *ctl) { + ctlcmdConfigAccountRemove(ctl, "mjl2") + }) + + // "domainrm" + testctl(func(ctl *ctl) { + ctlcmdConfigDomainRemove(ctl, dns.Domain{ASCII: "mox2.example"}) + }) + + // "loglevels" + testctl(func(ctl *ctl) { + ctlcmdLoglevels(ctl) + }) + + // "setloglevels" + testctl(func(ctl *ctl) { + ctlcmdSetLoglevels(ctl, "", "debug") + }) + testctl(func(ctl *ctl) { + ctlcmdSetLoglevels(ctl, "smtpserver", "debug") + }) + + // Export data, import it again + xcmdExport(true, []string{"testdata/ctl/data/tmp/export/mbox/", "testdata/ctl/data/accounts/mjl"}, nil) + xcmdExport(false, []string{"testdata/ctl/data/tmp/export/maildir/", "testdata/ctl/data/accounts/mjl"}, nil) + testctl(func(ctl *ctl) { + ctlcmdImport(ctl, true, "mjl", "inbox", "testdata/ctl/data/tmp/export/mbox/Inbox.mbox") + }) + testctl(func(ctl *ctl) { + ctlcmdImport(ctl, false, "mjl", "inbox", "testdata/ctl/data/tmp/export/maildir/Inbox") + }) + + // "backup", backup account. + err = dmarcdb.Init() + tcheck(t, err, "dmarcdb init") + err = mtastsdb.Init(false) + tcheck(t, err, "mtastsdb init") + err = tlsrptdb.Init() + tcheck(t, err, "tlsrptdb init") + testctl(func(ctl *ctl) { + os.RemoveAll("testdata/ctl/data/tmp/backup-data") + err := os.WriteFile("testdata/ctl/data/receivedid.key", make([]byte, 16), 0600) + tcheck(t, err, "writing receivedid.key") + ctlcmdBackup(ctl, "testdata/ctl/data/tmp/backup-data", false) + }) + + // Verify the backup. + xcmd := cmd{ + flag: flag.NewFlagSet("", flag.ExitOnError), + flagArgs: []string{"testdata/ctl/data/tmp/backup-data"}, + } + cmdVerifydata(&xcmd) +} diff --git a/import.go b/import.go index 2905da6..a6e900f 100644 --- a/import.go +++ b/import.go @@ -49,9 +49,12 @@ dovecot-keywords file can specify additional flags, like Forwarded/Junk/NotJunk. The maildir files/directories are read by the mox process, so make sure it has access to the maildir directories/files. ` - args := c.Parse() - xcmdImport(false, args, c) + if len(args) != 3 { + c.Usage() + } + mustLoadConfig() + ctlcmdImport(xctl(), false, args[0], args[1], args[2]) } func cmdImportMbox(c *cmd) { @@ -65,35 +68,12 @@ Using mbox is not recommended, maildir is a better defined format. The mailbox is read by the mox process, so make sure it has access to the maildir directories/files. ` - args := c.Parse() - xcmdImport(true, args, c) -} - -func xcmdImport(mbox bool, args []string, c *cmd) { if len(args) != 3 { c.Usage() } - mustLoadConfig() - - account := args[0] - mailbox := args[1] - if strings.EqualFold(mailbox, "inbox") { - mailbox = "Inbox" - } - src := args[2] - - var ctlcmd string - if mbox { - ctlcmd = "importmbox" - } else { - ctlcmd = "importmaildir" - } - - ctl := xctl() - ctl.xwrite(ctlcmd) - xcmdImportCtl(ctl, account, mailbox, src) + ctlcmdImport(xctl(), true, args[0], args[1], args[2]) } func cmdXImportMaildir(c *cmd) { @@ -124,19 +104,6 @@ func xcmdXImport(mbox bool, c *cmd) { } accountdir := args[0] - mailbox := args[1] - if strings.EqualFold(mailbox, "inbox") { - mailbox = "Inbox" - } - src := args[2] - - var ctlcmd string - if mbox { - ctlcmd = "importmbox" - } else { - ctlcmd = "importmaildir" - } - account := filepath.Base(accountdir) // Set up the mox config so the account can be opened. @@ -157,14 +124,22 @@ func xcmdXImport(mbox bool, c *cmd) { xlog := mlog.New("import") cconn, sconn := net.Pipe() clientctl := ctl{conn: cconn, r: bufio.NewReader(cconn), log: xlog} - serverctl := ctl{cmd: ctlcmd, conn: sconn, r: bufio.NewReader(sconn), log: xlog} - go importctl(context.Background(), &serverctl, true) + serverctl := ctl{conn: sconn, r: bufio.NewReader(sconn), log: xlog} + go servectlcmd(context.Background(), &serverctl, func() {}) - xcmdImportCtl(&clientctl, account, mailbox, src) + ctlcmdImport(&clientctl, mbox, account, args[1], args[2]) } -func xcmdImportCtl(ctl *ctl, account, mailbox, src string) { +func ctlcmdImport(ctl *ctl, mbox bool, account, mailbox, src string) { + if mbox { + ctl.xwrite("importmbox") + } else { + ctl.xwrite("importmaildir") + } ctl.xwrite(account) + if strings.EqualFold(mailbox, "Inbox") { + mailbox = "Inbox" + } ctl.xwrite(mailbox) ctl.xwrite(src) ctl.xreadok() diff --git a/integration_test.go b/integration_test.go index 6b03f70..7fc3658 100644 --- a/integration_test.go +++ b/integration_test.go @@ -10,7 +10,6 @@ import ( "net" "os" "os/exec" - "path/filepath" "strings" "testing" "time" @@ -26,10 +25,9 @@ import ( var ctxbg = context.Background() -func tcheck(t *testing.T, err error, msg string) { - t.Helper() +func tcheck(t *testing.T, err error, errmsg string) { if err != nil { - t.Fatalf("%s: %s", msg, err) + t.Fatalf("%s: %s", errmsg, err) } } @@ -49,7 +47,6 @@ func TestDeliver(t *testing.T) { // Load mox config. mox.ConfigStaticPath = "testdata/integration/config/mox.conf" - filepath.Join(filepath.Dir(mox.ConfigStaticPath), "domains.conf") if errs := mox.LoadConfig(ctxbg, true, false); len(errs) > 0 { t.Fatalf("loading mox config: %v", errs) } diff --git a/main.go b/main.go index 2c659ae..29e7c83 100644 --- a/main.go +++ b/main.go @@ -592,17 +592,20 @@ must be set if and only if account does not yet exist. d := xparseDomain(args[0], "domain") mustLoadConfig() + var localpart string + if len(args) == 3 { + localpart = args[2] + } + ctlcmdConfigDomainAdd(xctl(), d, args[1], localpart) +} - if len(args) == 2 { - args = append(args, "") - } - ctl := xctl() +func ctlcmdConfigDomainAdd(ctl *ctl, domain dns.Domain, account, localpart string) { ctl.xwrite("domainadd") - for _, s := range args { - ctl.xwrite(s) - } + ctl.xwrite(domain.Name()) + ctl.xwrite(account) + ctl.xwrite(localpart) ctl.xreadok() - fmt.Printf("domain added, remember to add dns records, see:\n\nmox config dnsrecords %s\nmox config dnscheck %s\n", d.Name(), d.Name()) + fmt.Printf("domain added, remember to add dns records, see:\n\nmox config dnsrecords %s\nmox config dnscheck %s\n", domain.Name(), domain.Name()) } func cmdConfigDomainRemove(c *cmd) { @@ -619,9 +622,12 @@ rejected. d := xparseDomain(args[0], "domain") mustLoadConfig() - ctl := xctl() + ctlcmdConfigDomainRemove(xctl(), d) +} + +func ctlcmdConfigDomainRemove(ctl *ctl, d dns.Domain) { ctl.xwrite("domainrm") - ctl.xwrite(args[0]) + ctl.xwrite(d.Name()) ctl.xreadok() fmt.Printf("domain removed, remember to remove dns records for %s\n", d) } @@ -639,13 +645,15 @@ explicitly, see the setaccountpassword command. } mustLoadConfig() - ctl := xctl() + ctlcmdConfigAccountAdd(xctl(), args[0], args[1]) +} + +func ctlcmdConfigAccountAdd(ctl *ctl, account, address string) { ctl.xwrite("accountadd") - for _, s := range args { - ctl.xwrite(s) - } + ctl.xwrite(account) + ctl.xwrite(address) ctl.xreadok() - fmt.Printf("account added, set a password with \"mox setaccountpassword %s\"\n", args[1]) + fmt.Printf("account added, set a password with \"mox setaccountpassword %s\"\n", address) } func cmdConfigAccountRemove(c *cmd) { @@ -661,9 +669,12 @@ these addresses will be rejected. } mustLoadConfig() - ctl := xctl() + ctlcmdConfigAccountRemove(xctl(), args[0]) +} + +func ctlcmdConfigAccountRemove(ctl *ctl, account string) { ctl.xwrite("accountrm") - ctl.xwrite(args[0]) + ctl.xwrite(account) ctl.xreadok() fmt.Println("account removed") } @@ -681,11 +692,13 @@ address for the domain. } mustLoadConfig() - ctl := xctl() + ctlcmdConfigAddressAdd(xctl(), args[0], args[1]) +} + +func ctlcmdConfigAddressAdd(ctl *ctl, address, account string) { ctl.xwrite("addressadd") - for _, s := range args { - ctl.xwrite(s) - } + ctl.xwrite(address) + ctl.xwrite(account) ctl.xreadok() fmt.Println("address added") } @@ -702,9 +715,12 @@ Incoming email for this address will be rejected after removing an address. } mustLoadConfig() - ctl := xctl() + ctlcmdConfigAddressRemove(xctl(), args[0]) +} + +func ctlcmdConfigAddressRemove(ctl *ctl, address string) { ctl.xwrite("addressrm") - ctl.xwrite(args[0]) + ctl.xwrite(address) ctl.xreadok() fmt.Println("address removed") } @@ -931,21 +947,26 @@ Valid labels: error, info, debug, trace, traceauth, tracedata. mustLoadConfig() if len(args) == 0 { - ctl := xctl() - ctl.xwrite("loglevels") - ctl.xreadok() - ctl.xstreamto(os.Stdout) - return - } - - ctl := xctl() - ctl.xwrite("setloglevels") - if len(args) == 2 { - ctl.xwrite(args[1]) + ctlcmdLoglevels(xctl()) } else { - ctl.xwrite("") + var pkg string + if len(args) == 2 { + pkg = args[1] + } + ctlcmdSetLoglevels(xctl(), pkg, args[0]) } - ctl.xwrite(args[0]) +} + +func ctlcmdLoglevels(ctl *ctl) { + ctl.xwrite("loglevels") + ctl.xreadok() + ctl.xstreamto(os.Stdout) +} + +func ctlcmdSetLoglevels(ctl *ctl, pkg, level string) { + ctl.xwrite("setloglevels") + ctl.xwrite(pkg) + ctl.xwrite(level) ctl.xreadok() } @@ -1025,7 +1046,10 @@ upgrading. dstDataDir, err := filepath.Abs(args[0]) xcheckf(err, "making path absolute") - ctl := xctl() + ctlcmdBackup(xctl(), dstDataDir, verbose) +} + +func ctlcmdBackup(ctl *ctl, dstDataDir string, verbose bool) { ctl.xwrite("backup") ctl.xwrite(dstDataDir) if verbose { @@ -1102,10 +1126,13 @@ Any email address configured for the account can be used. pw := xreadpassword() - ctl := xctl() + ctlcmdSetaccountpassword(xctl(), args[0], pw) +} + +func ctlcmdSetaccountpassword(ctl *ctl, address, password string) { ctl.xwrite("setaccountpassword") - ctl.xwrite(args[0]) - ctl.xwrite(pw) + ctl.xwrite(address) + ctl.xwrite(password) ctl.xreadok() } @@ -1118,10 +1145,12 @@ func cmdDeliver(c *cmd) { c.Usage() } mustLoadConfig() + ctlcmdDeliver(xctl(), args[0]) +} - ctl := xctl() +func ctlcmdDeliver(ctl *ctl, address string) { ctl.xwrite("deliver") - ctl.xwrite(args[0]) + ctl.xwrite(address) ctl.xreadok() ctl.xstreamfrom(os.Stdin) line := ctl.xread() @@ -1142,8 +1171,10 @@ error. c.Usage() } mustLoadConfig() + ctlcmdQueueList(xctl()) +} - ctl := xctl() +func ctlcmdQueueList(ctl *ctl) { ctl.xwrite("queue") ctl.xreadok() if _, err := io.Copy(os.Stdout, ctl.reader()); err != nil { @@ -1174,8 +1205,10 @@ queue over SMTP. c.Usage() } mustLoadConfig() + ctlcmdQueueKick(xctl(), id, todomain, recipient, transport) +} - ctl := xctl() +func ctlcmdQueueKick(ctl *ctl, id int64, todomain, recipient, transport string) { ctl.xwrite("queuekick") ctl.xwrite(fmt.Sprintf("%d", id)) ctl.xwrite(todomain) @@ -1206,8 +1239,10 @@ the message, use "queue dump" before removing. c.Usage() } mustLoadConfig() + ctlcmdQueueDrop(xctl(), id, todomain, recipient) +} - ctl := xctl() +func ctlcmdQueueDrop(ctl *ctl, id int64, todomain, recipient string) { ctl.xwrite("queuedrop") ctl.xwrite(fmt.Sprintf("%d", id)) ctl.xwrite(todomain) @@ -1232,10 +1267,12 @@ The message is printed to stdout and is in standard internet mail format. c.Usage() } mustLoadConfig() + ctlcmdQueueDump(xctl(), args[0]) +} - ctl := xctl() +func ctlcmdQueueDump(ctl *ctl, id string) { ctl.xwrite("queuedump") - ctl.xwrite(args[0]) + ctl.xwrite(id) ctl.xreadok() if _, err := io.Copy(os.Stdout, ctl.reader()); err != nil { log.Fatalf("%s", err) @@ -1786,9 +1823,12 @@ implementation has changed. } mustLoadConfig() - ctl := xctl() + ctlcmdRetrain(xctl(), args[0]) +} + +func ctlcmdRetrain(ctl *ctl, account string) { ctl.xwrite("retrain") - ctl.xwrite(args[0]) + ctl.xwrite(account) ctl.xreadok() } diff --git a/mox-/admin.go b/mox-/admin.go index 3fac963..f35d536 100644 --- a/mox-/admin.go +++ b/mox-/admin.go @@ -186,7 +186,7 @@ func MakeDomainConfig(ctx context.Context, domain, hostname dns.Domain, accountN addSelector := func(kind, name string, privKey []byte) error { record := fmt.Sprintf("%s._domainkey.%s", name, domain.ASCII) keyPath := filepath.Join("dkim", fmt.Sprintf("%s.%s.%skey.pkcs8.pem", record, timestamp, kind)) - p := ConfigDirPath(keyPath) + p := configDirPath(ConfigDynamicPath, keyPath) if err := writeFile(p, privKey); err != nil { return err } diff --git a/quickstart_test.go b/quickstart_test.go index 99010cf..080a0ac 100644 --- a/quickstart_test.go +++ b/quickstart_test.go @@ -5,6 +5,7 @@ package main import ( + "context" "crypto/tls" "fmt" "os" @@ -20,16 +21,16 @@ import ( "github.com/mjl-/mox/smtpclient" ) -var xlog = mlog.New("quickstart") +var ctxbg = context.Background() -func tcheck(t *testing.T, err error, msg string) { - t.Helper() +func tcheck(t *testing.T, err error, errmsg string) { if err != nil { - t.Fatalf("%s: %s", msg, err) + t.Fatalf("%s: %s", errmsg, err) } } func TestDeliver(t *testing.T) { + xlog := mlog.New("quickstart") mlog.Logfmt = true hostname, err := os.Hostname() diff --git a/testdata/ctl/domains.conf b/testdata/ctl/domains.conf new file mode 100644 index 0000000..449f11e --- /dev/null +++ b/testdata/ctl/domains.conf @@ -0,0 +1,7 @@ +Domains: + mox.example: nil +Accounts: + mjl: + Domain: mox.example + Destinations: + mjl@mox.example: nil diff --git a/testdata/ctl/mox.conf b/testdata/ctl/mox.conf new file mode 100644 index 0000000..e1286db --- /dev/null +++ b/testdata/ctl/mox.conf @@ -0,0 +1,9 @@ +DataDir: data +User: 1000 +LogLevel: trace +Hostname: mox.example +Postmaster: + Account: mjl + Mailbox: postmaster +Listeners: + local: nil