feat(cli): positional whitelist grammar with required direction, enable/disable, validation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-27 11:56:54 +01:00
parent f407fc126d
commit 1bf5bf3c47
2 changed files with 168 additions and 69 deletions
@@ -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 <in|out> 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 <add|remove|list|enable|disable> <account>
+// [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 <in|out> <add|remove|list> [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 <in|out> <add|remove|list> [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 <in|out> <add|remove|list> [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
 }
@@ -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)
 	}
 }