From 9a8765d4e4b5fba85cccb29b742081e23b66c7e0 Mon Sep 17 00:00:00 2001 From: Steve Cliff Date: Sat, 27 Jun 2026 12:04:04 +0100 Subject: [PATCH] feat(cli): positional account grammar, account show, TTY remove confirm; drop whitelist flags Co-Authored-By: Claude Sonnet 4.6 --- go.mod | 2 +- internal/cli/account_list_test.go | 6 +- internal/cli/account_show.go | 49 ++++++++++++ internal/cli/account_show_test.go | 36 +++++++++ internal/cli/admin.go | 129 +++++++++++++++++------------- internal/cli/admin_test.go | 27 ++++--- 6 files changed, 180 insertions(+), 69 deletions(-) create mode 100644 internal/cli/account_show.go create mode 100644 internal/cli/account_show_test.go diff --git a/go.mod b/go.mod index c52d0ac..4209412 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/emersion/go-message v0.18.2 github.com/emersion/go-sasl v0.0.0-20241020182733-b788ff22d5a6 github.com/emersion/go-smtp v0.24.0 + github.com/mattn/go-isatty v0.0.20 modernc.org/sqlite v1.53.0 ) @@ -27,7 +28,6 @@ require ( github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect github.com/google/uuid v1.6.0 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect - github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-localereader v0.0.1 // indirect github.com/mattn/go-runewidth v0.0.19 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect diff --git a/internal/cli/account_list_test.go b/internal/cli/account_list_test.go index 4770aa4..cddb5b6 100644 --- a/internal/cli/account_list_test.go +++ b/internal/cli/account_list_test.go @@ -10,10 +10,10 @@ import ( // name/from/can_send, and never the IMAP host or login username. func TestAccountListAgentJSONView(t *testing.T) { adminEnv(t) // both keys + initialized temp DB - run(t, "account", "add", "--name", "work", "--mode", "RW", + run(t, "account", "add", "work", "--mode", "RW", "--imap-host", "imap.example.com", "--smtp-host", "smtp.example.com", "--username", "login@example.com", "--from", "me@example.com") - run(t, "account", "add", "--name", "alerts", "--mode", "RO", + run(t, "account", "add", "alerts", "--mode", "RO", "--imap-host", "imap.example.com", "--username", "alerts@example.com") // Drop the admin key → caller is an agent. @@ -66,7 +66,7 @@ func TestAccountListAgentJSONView(t *testing.T) { // With the admin key present, `account list` stays the full human-readable table. func TestAccountListAdminTextView(t *testing.T) { adminEnv(t) - run(t, "account", "add", "--name", "work", "--mode", "RW", + run(t, "account", "add", "work", "--mode", "RW", "--imap-host", "imap.example.com", "--smtp-host", "smtp.example.com", "--username", "login@example.com", "--from", "me@example.com") diff --git a/internal/cli/account_show.go b/internal/cli/account_show.go new file mode 100644 index 0000000..c8b3918 --- /dev/null +++ b/internal/cli/account_show.go @@ -0,0 +1,49 @@ +package cli + +import ( + "fmt" + "io" + + "git.dcglab.co.uk/steve/emcli/internal/store" +) + +// accountShow renders one account's configuration. The password is never shown. +func accountShow(st *store.Store, rest []string, out, errOut io.Writer) int { + if len(rest) == 0 { + fmt.Fprintln(errOut, "usage: emcli account show ") + return 2 + } + if len(rest) > 1 { + fmt.Fprintf(errOut, "unexpected argument %q\n", rest[1]) + return 2 + } + a, err := st.GetAccount(rest[0]) + if err != nil { + fmt.Fprintf(errOut, "show: %v\n", err) + return 1 + } + onOff := func(b bool) string { + if b { + return "enabled" + } + return "disabled" + } + smtp := "-" + if a.SMTPHost != "" { + smtp = fmt.Sprintf("%s:%d (%s)", a.SMTPHost, a.SMTPPort, a.SMTPSecurity) + } + subj := a.SubjectRegex + if subj == "" { + subj = "(none)" + } + fmt.Fprintf(out, "name: %s\n", a.Name) + fmt.Fprintf(out, "mode: %s\n", a.Mode) + fmt.Fprintf(out, "imap: %s:%d (%s)\n", a.IMAPHost, a.IMAPPort, a.IMAPSecurity) + fmt.Fprintf(out, "smtp: %s\n", smtp) + fmt.Fprintf(out, "username: %s\n", a.Username) + fmt.Fprintf(out, "send-from: %s\n", a.SendFrom()) + fmt.Fprintf(out, "subject filter: %s\n", subj) + fmt.Fprintf(out, "inbound whitelist: %s\n", onOff(a.WhitelistInEnabled)) + fmt.Fprintf(out, "outbound whitelist:%s\n", onOff(a.WhitelistOutEnabled)) + return 0 +} diff --git a/internal/cli/account_show_test.go b/internal/cli/account_show_test.go new file mode 100644 index 0000000..fdfacd8 --- /dev/null +++ b/internal/cli/account_show_test.go @@ -0,0 +1,36 @@ +package cli + +import ( + "strings" + "testing" +) + +func TestAccountShow(t *testing.T) { + adminEnv(t) + run(t, "account", "add", "shown", "--mode", "RW", + "--imap-host", "imap.x.com", "--imap-port", "993", + "--smtp-host", "smtp.x.com", "--smtp-port", "465", + "--username", "u@x.com", "--password", "secret", "--from", "me@x.com") + code, out, _ := run(t, "account", "show", "shown") + if code != 0 { + t.Fatalf("show exit=%d", code) + } + for _, want := range []string{"shown", "RW", "imap.x.com:993", "smtp.x.com:465", "u@x.com", "me@x.com"} { + if !strings.Contains(out, want) { + t.Fatalf("show missing %q:\n%s", want, out) + } + } + if strings.Contains(out, "secret") { + t.Fatalf("show must never print the password:\n%s", out) + } +} + +func TestAccountShowMissingName(t *testing.T) { + adminEnv(t) + if code, _, _ := run(t, "account", "show"); code != 2 { + t.Fatal("show without a name must be a usage error") + } + if code, _, _ := run(t, "account", "show", "nope"); code == 0 { + t.Fatal("show of a missing account must be non-zero") + } +} diff --git a/internal/cli/admin.go b/internal/cli/admin.go index f1967b2..95c0fbb 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1,33 +1,45 @@ package cli import ( + "bufio" "flag" "fmt" "io" + "os" "strings" + "github.com/mattn/go-isatty" + "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" ) -// runAccount handles `account add|list`. Human-readable output (never JSON). +// confirmRemoval prompts on a TTY for a y/N answer. Non-TTY callers never reach +// here (the caller requires --yes when stdin is not a terminal). +func confirmRemoval(name string, out io.Writer) bool { + fmt.Fprintf(out, "Remove account %q? [y/N]: ", name) + line, _ := bufio.NewReader(os.Stdin).ReadString('\n') + line = strings.ToLower(strings.TrimSpace(line)) + return line == "y" || line == "yes" +} + +// runAccount handles `account `. Human-readable +// output (except the agent-only reduced-JSON branch of `list`). func runAccount(args []string, role store.Role, out, errOut io.Writer) int { if len(args) == 0 || helpRequested(args[0]) { printCmdUsage(out, "account") - fmt.Fprintln(out, "\nSubcommands: add, edit, remove, list") + fmt.Fprintln(out, "\nSubcommands: add, edit, remove, show, list") if len(args) > 0 { - return 0 // explicit --help + return 0 } return 2 } - sub, rest := args[0], args[1:] + sub := normalizeVerb(args[0]) + rest := args[1:] st, err := openStore(role) if err != nil { - // account list is an agent command (a JSON consumer), so its - // open/key failures are emitted as an envelope, like the other agent - // commands; the admin subcommands stay human-readable. if sub == "list" { _ = Failure(CodeConfig, err.Error()).Write(out) } else { @@ -39,17 +51,16 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { switch sub { case "add": - if len(rest) == 0 { // no flags → interactive TUI form + if len(rest) == 0 { // no args → interactive TUI form return addInteractive(st, tui.Fields{}, out, errOut) } // Peel a leading positional name (if present) before flag parsing. - var positionalName string + var name string if !strings.HasPrefix(rest[0], "-") { - positionalName, rest = rest[0], rest[1:] + name, rest = rest[0], rest[1:] } fs := flag.NewFlagSet("account add", flag.ContinueOnError) fs.SetOutput(errOut) - name := fs.String("name", "", "account name") mode := fs.String("mode", "RO", "RO|RW") host := fs.String("imap-host", "", "IMAP host") port := fs.Int("imap-port", 993, "IMAP port") @@ -61,18 +72,16 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { pass := fs.String("password", "", "login password") from := fs.String("from", "", "send-as address (blank = use username)") subj := fs.String("subject-regex", "", "inbound subject filter") - wlIn := fs.Bool("whitelist-in", false, "enable inbound whitelist") - wlOut := fs.Bool("whitelist-out", false, "enable outbound whitelist") backlog := fs.Bool("process-backlog", false, "treat existing mail as new") if err := fs.Parse(rest); err != nil { return 2 } - // Positional name takes precedence; fall back to --name flag. - if positionalName != "" { - *name = positionalName + if fs.NArg() > 0 { + fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0)) + return 2 } - if *name == "" || *host == "" || *user == "" { - fmt.Fprintln(errOut, "name, imap-host, and username are required") + if name == "" || *host == "" || *user == "" { + fmt.Fprintln(errOut, "name, --imap-host, and --username are required") return 2 } if err := tui.ValidFromAddress(*from); err != nil { @@ -80,26 +89,31 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { return 2 } acc := store.Account{ - Name: *name, Mode: *mode, IMAPHost: *host, IMAPPort: *port, IMAPSecurity: *sec, + Name: name, Mode: *mode, IMAPHost: *host, IMAPPort: *port, IMAPSecurity: *sec, AuthType: "password", Username: *user, Password: *pass, - FromAddress: *from, - SubjectRegex: *subj, WhitelistInEnabled: *wlIn, WhitelistOutEnabled: *wlOut, - ProcessBacklog: *backlog, + FromAddress: *from, SubjectRegex: *subj, ProcessBacklog: *backlog, } if *mode == "RW" { acc.SMTPHost, acc.SMTPPort, acc.SMTPSecurity = *smtpHost, *smtpPort, *smtpSec } - _, err := st.AddAccount(acc) - if err != nil { + if _, err := st.AddAccount(acc); err != nil { fmt.Fprintf(errOut, "add account: %v\n", err) return 1 } - fmt.Fprintf(out, "account %q added (%s)\n", *name, *mode) + fmt.Fprintf(out, "account %q added (%s)\n", name, *mode) return 0 case "edit": + if len(rest) == 0 || strings.HasPrefix(rest[0], "-") { + fmt.Fprintln(errOut, "usage: emcli account edit [flags]") + return 2 + } + name := rest[0] + flagArgs := rest[1:] + if len(flagArgs) == 0 { // only name → interactive prefilled form + return editInteractive(st, name, out, errOut) + } fs := flag.NewFlagSet("account edit", flag.ContinueOnError) fs.SetOutput(errOut) - name := fs.String("name", "", "account name (required)") mode := fs.String("mode", "", "RO|RW") host := fs.String("imap-host", "", "IMAP host") port := fs.Int("imap-port", 0, "IMAP port") @@ -111,26 +125,22 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { pass := fs.String("password", "", "login password (blank keeps existing)") from := fs.String("from", "", "send-as address (empty reverts to username)") subj := fs.String("subject-regex", "", "inbound subject filter") - if err := fs.Parse(rest); err != nil { + if err := fs.Parse(flagArgs); err != nil { return 2 } - if *name == "" { - fmt.Fprintln(errOut, "--name is required") + if fs.NArg() > 0 { + fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0)) return 2 } - if fs.NFlag() == 1 { // only --name → interactive TUI form, prefilled - return editInteractive(st, *name, out, errOut) - } if err := tui.ValidFromAddress(*from); err != nil { fmt.Fprintln(errOut, err) return 2 } - acc, err := st.GetAccount(*name) + acc, err := st.GetAccount(name) if err != nil { fmt.Fprintf(errOut, "edit: %v\n", err) return 1 } - // Overlay only the flags the user actually set. fs.Visit(func(f *flag.Flag) { switch f.Name { case "mode": @@ -157,40 +167,51 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { acc.SubjectRegex = *subj } }) - // acc.Password holds the existing (decrypted) password from GetAccount; the - // Visit above overwrites it only when --password was passed. UpdateAccount - // re-seals whatever non-empty value is present, so the password is preserved. if err := st.UpdateAccount(acc); err != nil { fmt.Fprintf(errOut, "edit: %v\n", err) return 1 } - fmt.Fprintf(out, "account %q updated\n", *name) + fmt.Fprintf(out, "account %q updated\n", name) return 0 case "remove": - fs := flag.NewFlagSet("account remove", flag.ContinueOnError) - fs.SetOutput(errOut) - name := fs.String("name", "", "account name (required)") - yes := fs.Bool("yes", false, "skip confirmation") - if err := fs.Parse(rest); err != nil { + if len(rest) == 0 || strings.HasPrefix(rest[0], "-") { + fmt.Fprintln(errOut, "usage: emcli account remove [--yes]") return 2 } - if *name == "" { - fmt.Fprintln(errOut, "--name is required") + name := rest[0] + fs := flag.NewFlagSet("account remove", flag.ContinueOnError) + fs.SetOutput(errOut) + yes := fs.Bool("yes", false, "skip confirmation") + if err := fs.Parse(rest[1:]); err != nil { + return 2 + } + if fs.NArg() > 0 { + fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0)) return 2 } if !*yes { - fmt.Fprintf(errOut, "refusing to remove %q without --yes\n", *name) - return 2 + if !isatty.IsTerminal(os.Stdin.Fd()) { + fmt.Fprintf(errOut, "refusing to remove %q without --yes (no terminal for confirmation)\n", name) + return 2 + } + if !confirmRemoval(name, out) { + fmt.Fprintln(out, "aborted") + return 1 + } } - if err := st.DeleteAccount(*name); err != nil { + if err := st.DeleteAccount(name); err != nil { fmt.Fprintf(errOut, "remove: %v\n", err) return 1 } - fmt.Fprintf(out, "account %q removed\n", *name) + fmt.Fprintf(out, "account %q removed\n", name) return 0 + case "show": + return accountShow(st, rest, out, errOut) case "list": - // Holding the admin key means the caller is the human admin (full - // detail). An agent holds only EMCLI_KEY and gets a reduced JSON view. + if len(rest) > 0 { + fmt.Fprintf(errOut, "unexpected argument %q\n", rest[0]) + return 2 + } _, adminErr := crypto.AdminKeyFromEnv() isAdmin := adminErr == nil accs, err := st.ListAccounts() @@ -206,9 +227,7 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { items := make([]map[string]any, 0, len(accs)) for _, a := range accs { items = append(items, map[string]any{ - "name": a.Name, - "from": a.SendFrom(), - "can_send": a.Mode == "RW", + "name": a.Name, "from": a.SendFrom(), "can_send": a.Mode == "RW", }) } _ = Success(map[string]any{"accounts": items}).Write(out) @@ -221,7 +240,7 @@ func runAccount(args []string, role store.Role, out, errOut io.Writer) int { } return 0 default: - fmt.Fprintf(errOut, "unknown account subcommand %q\n", sub) + fmt.Fprintf(errOut, "unknown account subcommand %q (want add|edit|remove|show|list)\n", sub) return 2 } } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index bfa72fb..6c17a29 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -84,8 +84,8 @@ func TestConfigList(t *testing.T) { func TestAccountRemove(t *testing.T) { adminEnv(t) - run(t, "account", "add", "--name", "gone", "--imap-host", "h", "--username", "u@x.com") - if code, _, e := run(t, "account", "remove", "--name", "gone", "--yes"); code != 0 { + run(t, "account", "add", "gone", "--imap-host", "h", "--username", "u@x.com") + if code, _, e := run(t, "account", "remove", "gone", "--yes"); code != 0 { t.Fatalf("remove failed: %s", e) } _, out, _ := run(t, "account", "list") @@ -96,17 +96,26 @@ func TestAccountRemove(t *testing.T) { func TestAccountRemoveMissing(t *testing.T) { adminEnv(t) - if code, _, _ := run(t, "account", "remove", "--name", "nope", "--yes"); code == 0 { + if code, _, _ := run(t, "account", "remove", "nope", "--yes"); code == 0 { t.Fatal("removing a missing account must be non-zero") } } +func TestAccountRemoveNoTTYNeedsYes(t *testing.T) { + adminEnv(t) + run(t, "account", "add", "keep", "--imap-host", "h", "--username", "u@x.com") + // Under `go test`, stdin is not a TTY, so without --yes this must refuse. + code, _, errOut := run(t, "account", "remove", "keep") + if code != 2 || !strings.Contains(errOut, "--yes") { + t.Fatalf("non-TTY remove without --yes must refuse: code=%d err=%q", code, errOut) + } +} + func TestAccountEditPartialPreservesOtherFields(t *testing.T) { db := adminEnv(t) - run(t, "account", "add", "--name", "ed", "--mode", "RO", + run(t, "account", "add", "ed", "--mode", "RO", "--imap-host", "imap.x.com", "--username", "u@x.com", "--password", "orig") - // Edit only mode + add SMTP; imap-host, username, password must be preserved. - if code, _, e := run(t, "account", "edit", "--name", "ed", "--mode", "RW", + if code, _, e := run(t, "account", "edit", "ed", "--mode", "RW", "--smtp-host", "smtp.x.com", "--smtp-port", "587", "--smtp-security", "starttls"); code != 0 { t.Fatalf("edit failed: %s", e) } @@ -217,10 +226,8 @@ func TestAuditListCoreRenders(t *testing.T) { func TestAccountEditFromValidationRejectsMalformed(t *testing.T) { adminEnv(t) - // Seed an account so the failure is from --from validation, not a missing account. - run(t, "account", "add", "--name", "valacc", "--imap-host", "imap.x.com", "--username", "u@x.com") - // A malformed --from value must be rejected with exit code 2 before touching the account. - code, _, errStr := run(t, "account", "edit", "--name", "valacc", "--from", "not an address") + run(t, "account", "add", "valacc", "--imap-host", "imap.x.com", "--username", "u@x.com") + code, _, errStr := run(t, "account", "edit", "valacc", "--from", "not an address") if code != 2 { t.Fatalf("expected exit code 2 for malformed --from, got %d (stderr: %q)", code, errStr) }