From 1bf5bf3c47576205662cbcc32472c7df3dbb0eca Mon Sep 17 00:00:00 2001 From: Steve Cliff Date: Sat, 27 Jun 2026 11:56:54 +0100 Subject: [PATCH] feat(cli): positional whitelist grammar with required direction, enable/disable, validation Co-Authored-By: Claude Sonnet 4.6 --- internal/cli/admin.go | 151 ++++++++++++++++++++++++++++--------- internal/cli/admin_test.go | 86 ++++++++++++--------- 2 files changed, 168 insertions(+), 69 deletions(-) diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 9c854dd..f1967b2 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -4,8 +4,10 @@ import ( "flag" "fmt" "io" + "strings" "git.dcglab.co.uk/steve/emcli/internal/crypto" + "git.dcglab.co.uk/steve/emcli/internal/policy" "git.dcglab.co.uk/steve/emcli/internal/store" "git.dcglab.co.uk/steve/emcli/internal/tui" ) @@ -40,6 +42,11 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { if len(rest) == 0 { // no flags → interactive TUI form return addInteractive(st, tui.Fields{}, out, errOut) } + // Peel a leading positional name (if present) before flag parsing. + var positionalName string + if !strings.HasPrefix(rest[0], "-") { + positionalName, rest = rest[0], rest[1:] + } fs := flag.NewFlagSet("account add", flag.ContinueOnError) fs.SetOutput(errOut) name := fs.String("name", "", "account name") @@ -60,6 +67,10 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { if err := fs.Parse(rest); err != nil { return 2 } + // Positional name takes precedence; fall back to --name flag. + if positionalName != "" { + *name = positionalName + } if *name == "" || *host == "" || *user == "" { fmt.Fprintln(errOut, "name, imap-host, and username are required") return 2 @@ -339,7 +350,16 @@ func runAudit(args []string, role store.Role, out, errOut io.Writer) int { return 0 } -// runWhitelist handles `whitelist add --account NAME --address A`. +// flowName renders a direction for human-facing prose. +func flowName(dir store.Direction) string { + if dir == store.DirOut { + return "outbound" + } + return "inbound" +} + +// runWhitelist handles `whitelist +// [addr…] --in|--out`. func runWhitelist(args []string, role store.Role, out, errOut io.Writer) int { if len(args) == 0 || helpRequested(args[0]) { printCmdUsage(out, "whitelist") @@ -348,35 +368,42 @@ func runWhitelist(args []string, role store.Role, out, errOut io.Writer) int { } return 2 } - if len(args) < 2 { - fmt.Fprintln(errOut, "usage: emcli whitelist [flags]") - return 2 - } - dir := store.Direction(args[0]) - if dir != store.DirIn && dir != store.DirOut { - fmt.Fprintf(errOut, "whitelist direction must be \"in\" or \"out\", got %q\n", args[0]) - fmt.Fprintln(errOut, "usage: emcli whitelist [flags]") - return 2 - } - sub, rest := args[1], args[2:] + sub := normalizeVerb(args[0]) switch sub { - case "add", "remove", "list": // valid + case "add", "remove", "list", "enable", "disable": // valid default: - fmt.Fprintf(errOut, "unknown whitelist subcommand %q (want add|remove|list)\n", sub) - fmt.Fprintln(errOut, "usage: emcli whitelist [flags]") + fmt.Fprintf(errOut, "unknown whitelist subcommand %q (want add|remove|list|enable|disable)\n", sub) return 2 } - fs := flag.NewFlagSet("whitelist", flag.ContinueOnError) - fs.SetOutput(errOut) - account := fs.String("account", "", "account name") - address := fs.String("address", "", "email or @domain") - if err := fs.Parse(rest); err != nil { + + // Split the remaining tokens into the direction flag and positionals. + var dir store.Direction + var dirSet bool + var pos []string + for _, a := range args[1:] { + switch a { + case "--in", "-in": + dir, dirSet = store.DirIn, true + case "--out", "-out": + dir, dirSet = store.DirOut, true + default: + if strings.HasPrefix(a, "-") { + fmt.Fprintf(errOut, "unknown flag %q (use --in or --out)\n", a) + return 2 + } + pos = append(pos, a) + } + } + if !dirSet { + fmt.Fprintln(errOut, "direction is required: pass --in or --out") return 2 } - if *account == "" { - fmt.Fprintln(errOut, "--account is required") + if len(pos) == 0 { + fmt.Fprintln(errOut, "account is required") return 2 } + account, addrs := pos[0], pos[1:] + st, err := openStore(role) if err != nil { fmt.Fprintf(errOut, "emcli: %v\n", err) @@ -385,27 +412,83 @@ func runWhitelist(args []string, role store.Role, out, errOut io.Writer) int { defer st.Close() switch sub { - case "add": - if err := st.AddWhitelist(*account, dir, *address); err != nil { - fmt.Fprintf(errOut, "add: %v\n", err) - return 1 + case "add", "remove": + if len(addrs) == 0 { + fmt.Fprintln(errOut, "at least one address is required") + return 2 } - fmt.Fprintf(out, "added %s to %s whitelist of %q\n", *address, dir, *account) - case "remove": - if err := st.RemoveWhitelist(*account, dir, *address); err != nil { - fmt.Fprintf(errOut, "remove: %v\n", err) - return 1 + for _, addr := range addrs { + if err := policy.ValidWhitelistAddress(addr); err != nil { + fmt.Fprintln(errOut, err) + return 2 + } } - fmt.Fprintf(out, "removed %s\n", *address) + for _, addr := range addrs { + if sub == "add" { + err = st.AddWhitelist(account, dir, addr) + } else { + err = st.RemoveWhitelist(account, dir, addr) + } + if err != nil { + fmt.Fprintf(errOut, "%s: %v\n", sub, err) + return 1 + } + } + verb := "added" + if sub == "remove" { + verb = "removed" + } + fmt.Fprintf(out, "%s %d address(es) in the %s whitelist of %q\n", verb, len(addrs), dir, account) + return 0 case "list": - addrs, err := st.ListWhitelist(*account, dir) + if len(addrs) > 0 { + fmt.Fprintf(errOut, "unexpected argument %q\n", addrs[0]) + return 2 + } + acc, err := st.GetAccount(account) if err != nil { fmt.Fprintf(errOut, "list: %v\n", err) return 1 } - for _, a := range addrs { + enabled := acc.WhitelistInEnabled + if dir == store.DirOut { + enabled = acc.WhitelistOutEnabled + } + state := "DISABLED" + if enabled { + state = "ENABLED" + } + entries, err := st.ListWhitelist(account, dir) + if err != nil { + fmt.Fprintf(errOut, "list: %v\n", err) + return 1 + } + fmt.Fprintf(out, "%s whitelist of %q: %s\n", dir, account, state) + for _, a := range entries { fmt.Fprintln(out, a) } + return 0 + case "enable", "disable": + if len(addrs) > 0 { + fmt.Fprintf(errOut, "unexpected argument %q\n", addrs[0]) + return 2 + } + enable := sub == "enable" + if enable { + if entries, _ := st.ListWhitelist(account, dir); len(entries) == 0 { + fmt.Fprintf(errOut, "warning: %s whitelist for %q is empty — this blocks ALL %s mail until you add addresses\n", dir, account, flowName(dir)) + } + } + if err := st.SetWhitelistEnabled(account, dir, enable); err != nil { + fmt.Fprintf(errOut, "%s: %v\n", sub, err) + return 1 + } + state := "disabled" + if enable { + state = "enabled" + } + fmt.Fprintf(out, "%s whitelist of %q %s\n", dir, account, state) + return 0 } return 0 } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 929ae7a..bfa72fb 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -132,48 +132,64 @@ func TestAccountEditPartialPreservesOtherFields(t *testing.T) { } } -// A missing direction (e.g. `whitelist list`) must report the real problem — -// the in|out direction — not the misleading "--account is required". -func TestWhitelistMissingDirectionReported(t *testing.T) { +func TestWhitelistRequiresDirection(t *testing.T) { adminEnv(t) - code, _, errOut := run(t, "whitelist", "list", "--account", "bobby") - if code == 0 { - t.Fatal("missing direction must be a usage error") - } - if strings.Contains(errOut, "--account is required") { - t.Fatalf("misleading error; want a direction complaint, got: %q", errOut) - } - if !strings.Contains(errOut, "in") || !strings.Contains(errOut, "out") { - t.Fatalf("error should name the in|out direction, got: %q", errOut) + run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com") + code, _, errOut := run(t, "whitelist", "add", "bobby", "a@x.com") + if code != 2 || !strings.Contains(errOut, "--in") || !strings.Contains(errOut, "--out") { + t.Fatalf("missing direction must name --in/--out: code=%d err=%q", code, errOut) } } -// A missing subcommand (e.g. `whitelist out --account x`) must report the real -// problem — the add|remove|list subcommand — not "--account is required". -func TestWhitelistMissingSubcommandReported(t *testing.T) { +func TestWhitelistAddListRemove(t *testing.T) { adminEnv(t) - code, _, errOut := run(t, "whitelist", "out", "--account", "bobby") - if code == 0 { - t.Fatal("missing subcommand must be a usage error") - } - if strings.Contains(errOut, "--account is required") { - t.Fatalf("misleading error; want a subcommand complaint, got: %q", errOut) - } - if !strings.Contains(errOut, "add") || !strings.Contains(errOut, "list") { - t.Fatalf("error should name the add|remove|list subcommand, got: %q", errOut) - } -} - -// The happy path still works after the direction/subcommand validation. -func TestWhitelistListWorks(t *testing.T) { - adminEnv(t) - run(t, "account", "add", "--name", "bobby", "--imap-host", "h", "--username", "u@x.com") - if code, _, e := run(t, "whitelist", "out", "add", "--account", "bobby", "--address", "@x.com"); code != 0 { + run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com") + if code, _, e := run(t, "whitelist", "add", "bobby", "a@x.com", "@y.com", "--out"); code != 0 { t.Fatalf("add failed: %s", e) } - code, out, _ := run(t, "whitelist", "out", "list", "--account", "bobby") - if code != 0 || !strings.Contains(out, "@x.com") { - t.Fatalf("list: code=%d out=%q", code, out) + code, out, _ := run(t, "whitelist", "list", "bobby", "--out") + if code != 0 || !strings.Contains(out, "a@x.com") || !strings.Contains(out, "@y.com") || !strings.Contains(out, "DISABLED") { + t.Fatalf("list wrong: code=%d out=%q", code, out) + } + if code, _, e := run(t, "whitelist", "rm", "bobby", "a@x.com", "--out"); code != 0 { // alias + t.Fatalf("rm failed: %s", e) + } + _, out, _ = run(t, "whitelist", "ls", "bobby", "--out") // alias + if strings.Contains(out, "a@x.com") { + t.Fatalf("address not removed:\n%s", out) + } +} + +func TestWhitelistRejectsBadAddress(t *testing.T) { + adminEnv(t) + run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com") + if code, _, e := run(t, "whitelist", "add", "bobby", "notanaddress", "--in"); code != 2 || !strings.Contains(e, "invalid address") { + t.Fatalf("bad address must be rejected: code=%d err=%q", code, e) + } + // The original bug: a missing address must not silently insert a blank row. + if code, _, _ := run(t, "whitelist", "add", "bobby", "--in"); code != 2 { + t.Fatal("add with no address must be a usage error") + } +} + +func TestWhitelistEnableDisable(t *testing.T) { + adminEnv(t) + run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com") + // Enabling an empty whitelist warns but succeeds. + code, _, errOut := run(t, "whitelist", "enable", "bobby", "--in") + if code != 0 || !strings.Contains(errOut, "empty") { + t.Fatalf("enable empty: code=%d err=%q", code, errOut) + } + _, out, _ := run(t, "whitelist", "list", "bobby", "--in") + if !strings.Contains(out, "ENABLED") { + t.Fatalf("expected ENABLED:\n%s", out) + } + if code, _, e := run(t, "whitelist", "disable", "bobby", "--in"); code != 0 { + t.Fatalf("disable failed: %s", e) + } + _, out, _ = run(t, "whitelist", "list", "bobby", "--in") + if !strings.Contains(out, "DISABLED") { + t.Fatalf("expected DISABLED:\n%s", out) } }