diff --git a/CHANGELOG.md b/CHANGELOG.md index bd3f9ea..50de8a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Cross-module convention alignment, post-#122-review parity sweep: + - `account.AccountList` / `mailaccount.MailAccountList` now render + the `used_*_space` column with a `" MB"` suffix as part of the + value (matching the singular detail row) and use a bare `USED` + header. Consistent with the database slice after #122 followups + — all three modules now share a single unit-rendering convention. + - `ddns.DDNSUser.in_progress` is no longer marked `omitempty`, + aligning with the majority of read modules. Captured fixtures + have always carried `in_progress`; the empty-string fallback on + an older account is harmless. + - `cronjob.Client.Add`, `ddns.Client.Add`, `ftpuser.Client.Add`, + `sambauser.Client.Add`, `mailinglist.Client.Add` and + `mailforward.Client.Add` now emit per-field validation errors + (`requires a non-empty `) instead of a single combined + message ("requires a non-empty X, Y and Z"). A caller hitting + the domain validator can now tell which field actually broke, + matching the convention introduced by the database slice in + #122 followups. +- `docs/usage/destructive-writes.md` refactored: a single + "per-slice baseline" section captures the contract every wired + slice carries (gating policy, dry-run/audit/redaction, + generated-login printing) and a "per-slice deviations" table + calls out only what each slice changes (mailforward's target-list + phrasing, database's louder delete verb + optional + `--allowed-hosts` wildcard, ddns's no-`_new_password` split, …). + Replaces eight near-identical paragraphs that had grown to + roughly copy-paste. + - `kasapi-cli databases add` and `… update` now bind disjoint flag sets (mirroring the ddnsuser slice). The flag names are identical on both subcommands, but the help text of each reflects its own diff --git a/docs/usage/destructive-writes.md b/docs/usage/destructive-writes.md index 577ebbe..4385e97 100644 --- a/docs/usage/destructive-writes.md +++ b/docs/usage/destructive-writes.md @@ -10,85 +10,40 @@ This page documents the behavioural contract (issue v0.2.0 write phase, [#13](https://github.com/chmmou/kasapi-cli/issues/13)). The read commands are non-destructive and are not gated. -The first write endpoints — -[`mail forwards`](https://github.com/chmmou/kasapi-cli/issues/115) -`add` / `update` / `delete` — wire this contract. `delete` and -`update` are gated by the confirmation prompt (an `update_mailforward` -replaces the whole target list and is therefore irreversible); `add` -creates and is reversible, so it is **not** prompted. All three honour -`--dry-run` and emit an audit record. - -[`mail lists`](https://github.com/chmmou/kasapi-cli/issues/117) `add` / -`update` / `delete` wire the same contract with the same policy: -`update` and `delete` are gated; `add` is reversible and not prompted. -`update` is gated because the fields it sets — subscriber, -restrict-post and config — are each replaced wholesale (the -confirmation prompt phrases this as replacing the list's *settings*). -The list password passed to `add` is redacted in the `--dry-run` -preview and the audit record. - -[`cronjobs`](https://github.com/chmmou/kasapi-cli/issues/118) `add` / -`update` / `delete` wire the same contract with the same policy: -`update` and `delete` are gated; `add` is reversible and not prompted. -`update` is gated because every field it sets is replaced wholesale -(the confirmation prompt phrases this as replacing the cronjob's -*settings*) and `update` sends only the explicitly-changed flags. Any -`--http-password` passed to `add`/`update` is redacted in the -`--dry-run` preview and the audit record. - -[`ftpusers`](https://github.com/chmmou/kasapi-cli/issues/119) `add` / -`update` / `delete` wire the same contract with the same policy: -`update` and `delete` are gated; `add` is reversible and not prompted. -`update` is gated because every field it sets is replaced wholesale -(the confirmation prompt phrases this as replacing the FTP user's -*settings*) and `update` sends only the explicitly-changed flags. -`add_ftpuser` takes no login — KAS generates it and the command prints -it on success. The `--password` flag is redacted in the `--dry-run` -preview and the audit record for both `add` and `update` (it maps to -`ftp_password` on add, `ftp_new_password` on update). - -[`sambausers`](https://github.com/chmmou/kasapi-cli/issues/120) `add` / -`update` / `delete` wire the same contract with the same policy: -`update` and `delete` are gated; `add` is reversible and not prompted. -`update` is gated because every field it sets is replaced wholesale -(the confirmation prompt phrases this as replacing the Samba user's -*settings*) and `update` sends only the explicitly-changed flags. -`add_sambauser` takes no login — KAS generates it and the command -prints it on success. The `--password` flag is redacted in the -`--dry-run` preview and the audit record for both `add` and `update` -(it maps to `samba_password` on add, `samba_new_password` on update). - -[`databases`](https://github.com/chmmou/kasapi-cli/issues/122) `add` / -`update` / `delete` wire the same contract with one important -loudness adjustment: `delete_database`'s confirmation prompt uses the -verb `"permanently delete"` rather than the bare `"delete"` every -other slice uses, because dropping a database also drops every row -it contains — the loudest data-loss surface of the v0.2.0 write -phase. `update` and `delete` are gated; `add` is reversible and not -prompted. `update` is gated because every field it sets is replaced -wholesale (the confirmation prompt phrases this as replacing the -database's *settings*) and `update` sends only the explicitly-changed -flags. `add_database` takes no login — KAS generates it (the login -equals the database name on creation, e.g. `d0123460`) and the -command prints it on success. The `--password` flag is redacted in -the `--dry-run` preview and the audit record for both `add` and -`update` (it maps to `database_password` on add, -`database_new_password` on update). - -[`ddnsusers`](https://github.com/chmmou/kasapi-cli/issues/121) `add` / -`update` / `delete` wire the same contract with the same policy: -`update` and `delete` are gated; `add` is reversible and not prompted. -`update` is gated because every field it sets is replaced wholesale -(the confirmation prompt phrases this as replacing the DDNS user's -*settings*) and `update` sends only the explicitly-changed flags. -`add_ddnsuser` takes no login — KAS generates it and the command -prints it on success. Unlike the FTP/Samba slices there is no -`_new_password` split: `--password` maps to `dyndns_password` on both -`add` and `update`, and is redacted in the `--dry-run` preview and the -audit record. `update_ddnsuser` accepts `--target-ipv4` / -`--target-ipv6` instead of `add`'s legacy `--target-ip`; the ipv4/ipv6 -keys are undocumented in the KAS API but verified to work against the -live system (the captured update request fixture is authoritative). +## Per-slice baseline + +Every wired write slice carries the same baseline policy. Listing it +once here keeps the per-slice notes below short — they only call out +the deviations. + +- `add` is reversible (creates a new resource) → **not** gated; no + prompt. +- `update` and `delete` are irreversible (wholesale field replacement + on `update`; resource removal on `delete`) → **gated** by the #109 + prompt. The prompt phrases `update` as replacing the resource's + *settings*. +- `update` sends **only the explicitly-changed flags** (keyed on + cobra `Changed`), so an empty value is a deliberate "clear", not + "leave unchanged". +- All three subcommands honour `--dry-run` (#132) and emit a #131 + audit record. Secret request parameters are redacted in **both** + sinks. +- Subcommands whose KAS action generates the login server-side + (`add_ftpuser`, `add_sambauser`, `add_ddnsuser`, `add_database`, + `add_mailinglist`, `add_mailforward`) print the generated identifier + on success. + +## Per-slice deviations + +| Slice (issue) | Deviation from the baseline above | +|---|---| +| [`mail forwards`](https://github.com/chmmou/kasapi-cli/issues/115) | First slice to wire #109/#131/#132. `update_mailforward` replaces the whole *target list*; the prompt phrases this as replacing the forward's *targets* (not "settings"). | +| [`mail lists`](https://github.com/chmmou/kasapi-cli/issues/117) | The list password passed to `add` is redacted in the dry-run preview and audit record. | +| [`cronjobs`](https://github.com/chmmou/kasapi-cli/issues/118) | Any `--http-password` passed to `add`/`update` is redacted. | +| [`ftpusers`](https://github.com/chmmou/kasapi-cli/issues/119) | Password key splits between actions: `--password` → `ftp_password` on add, `ftp_new_password` on update. | +| [`sambausers`](https://github.com/chmmou/kasapi-cli/issues/120) | Password key splits between actions: `--password` → `samba_password` on add, `samba_new_password` on update. Note: the KAS docs wrongly list `samba_new_password` for the create call; the captured fixture confirms the real key is `samba_password`. | +| [`databases`](https://github.com/chmmou/kasapi-cli/issues/122) | **Louder delete prompt**: `delete_database` uses the verb `"permanently delete"` (vs the bare `"delete"` every other slice uses) because the action drops the database AND every row in it — the loudest data-loss surface of the v0.2.0 write phase. Password key splits between actions: `--password` → `database_password` on add, `database_new_password` on update. `--allowed-hosts` is **optional**: an empty value is the KAS API's documented "any host may connect" wildcard, sent verbatim on the wire. | +| [`ddnsusers`](https://github.com/chmmou/kasapi-cli/issues/121) | **No `_new_password` split**: `--password` maps to `dyndns_password` on both `add` and `update`. `update_ddnsuser` accepts `--target-ipv4` / `--target-ipv6` instead of `add`'s legacy `--target-ip`; the ipv4/ipv6 keys are undocumented in the KAS API docs but verified to work against the live system (the captured update request fixture is authoritative). | ## The contract diff --git a/internal/account/table.go b/internal/account/table.go index 5970623..6c05cfe 100644 --- a/internal/account/table.go +++ b/internal/account/table.go @@ -14,14 +14,17 @@ type AccountList []Account // TableHeaders returns the column names for the account list view. func (AccountList) TableHeaders() []string { - return []string{"LOGIN", "COMMENT", "MAIL", "MAX_DOMAIN", "MAX_WEBSPACE", "USED_MB", "2FA", "IN_PROGRESS"} + return []string{"LOGIN", "COMMENT", "MAIL", "MAX_DOMAIN", "MAX_WEBSPACE", "USED", "2FA", "IN_PROGRESS"} } // TableRows returns one row per account, in the order returned by KAS. // KAS returns used_account_space in KiB (the phpdoc at // https://kasapi.kasserver.com/dokumentation/phpdoc/ does not state the // unit, but real responses have magnitudes and fractional digits -// consistent with bytes/1024); /1024 yields MiB, displayed as MB. +// consistent with bytes/1024); /1024 yields MiB, displayed as MB. The +// unit travels with the value (matching the singular detail row and +// the cross-module convention) so list and detail views share a +// single unit-rendering rule. func (l AccountList) TableRows() [][]string { rows := make([][]string, 0, len(l)) for _, a := range l { @@ -31,7 +34,7 @@ func (l AccountList) TableRows() [][]string { a.AccountContactMail, strconv.Itoa(a.MaxDomain), strconv.Itoa(a.MaxWebspace), - strconv.FormatFloat(a.UsedAccountSpace/1024, 'f', 1, 64), + strconv.FormatFloat(a.UsedAccountSpace/1024, 'f', 1, 64) + " MB", a.Account2FA, a.InProgress, }) diff --git a/internal/cronjob/write.go b/internal/cronjob/write.go index 50123da..8fc234a 100644 --- a/internal/cronjob/write.go +++ b/internal/cronjob/write.go @@ -80,11 +80,23 @@ type Spec struct { // (minute_syntax_incorrect, time_not_allowed, …) surface verbatim // through the Caller. func (cl *Client) Add(ctx context.Context, s Spec) (string, error) { - if s.Protocol == "" || s.HTTPURL == "" || s.Comment == "" { - return "", errors.New("cronjob: add_cronjob requires a non-empty protocol, http url and comment") - } - if s.Minute == "" || s.Hour == "" || s.DayOfMonth == "" || s.Month == "" || s.DayOfWeek == "" { - return "", errors.New("cronjob: add_cronjob requires --minute and --hour (and non-empty day_of_month/month/day_of_week)") + switch { + case s.Protocol == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty protocol") + case s.HTTPURL == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty http url") + case s.Comment == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty comment") + case s.Minute == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty minute schedule") + case s.Hour == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty hour schedule") + case s.DayOfMonth == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty day_of_month schedule") + case s.Month == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty month schedule") + case s.DayOfWeek == "": + return "", errors.New("cronjob: add_cronjob requires a non-empty day_of_week schedule") } resp, err := kaswrite.Call(ctx, cl.c, "cronjob", addAction, AddParams(s)) if err != nil { diff --git a/internal/ddns/ddns.go b/internal/ddns/ddns.go index 91f3da0..739a3b6 100644 --- a/internal/ddns/ddns.go +++ b/internal/ddns/ddns.go @@ -28,8 +28,13 @@ type Caller = kasread.Caller // `dyndns_target_ip` alongside the explicit `dyndns_target_ipv4` and // `dyndns_target_ipv6` dual-stack fields (see // testdata/ddns/get_ddnsuser*_response_success.xml). The ipv4/ipv6 -// pair plus `in_progress` may still be absent on older accounts, so -// all three are flagged with omitempty. +// pair may still be absent on older accounts, so those two carry +// omitempty. +// +// `in_progress` is no longer omitempty for parity with the majority +// of read modules (account, mailaccount, mailinglist, sambauser, +// ftpuser, database): every captured fixture row carries it and a +// stray empty-string on an older account is harmless. type DDNSUser struct { Login string `json:"dyndns_login" yaml:"dyndns_login"` Password string `json:"dyndns_password,omitempty" yaml:"dyndns_password,omitempty"` @@ -43,7 +48,7 @@ type DDNSUser struct { DualStack string `json:"dyndns_dual_stack" yaml:"dyndns_dual_stack"` Comment string `json:"dyndns_comment" yaml:"dyndns_comment"` - InProgress string `json:"in_progress,omitempty" yaml:"in_progress,omitempty"` + InProgress string `json:"in_progress" yaml:"in_progress"` } // FQDN returns the fully-qualified DDNS hostname (label + zone) for diff --git a/internal/ddns/write.go b/internal/ddns/write.go index f94727c..efeafc7 100644 --- a/internal/ddns/write.go +++ b/internal/ddns/write.go @@ -85,8 +85,17 @@ type Spec struct { // dyndns_target_ip_syntax_incorrect, …) is left to the API and surfaces // verbatim through the Caller. func (cl *Client) Add(ctx context.Context, s Spec) (string, error) { - if s.Password == "" || s.Zone == "" || s.Label == "" || s.TargetIP == "" || s.Comment == "" { - return "", errors.New("ddns: add_ddnsuser requires a non-empty password, zone, label, target IP and comment") + switch { + case s.Password == "": + return "", errors.New("ddns: add_ddnsuser requires a non-empty password") + case s.Zone == "": + return "", errors.New("ddns: add_ddnsuser requires a non-empty zone") + case s.Label == "": + return "", errors.New("ddns: add_ddnsuser requires a non-empty label") + case s.TargetIP == "": + return "", errors.New("ddns: add_ddnsuser requires a non-empty target IP") + case s.Comment == "": + return "", errors.New("ddns: add_ddnsuser requires a non-empty comment") } resp, err := kaswrite.Call(ctx, cl.c, "ddns", addAction, AddParams(s)) if err != nil { diff --git a/internal/ftpuser/write.go b/internal/ftpuser/write.go index f9b15c7..d06c91f 100644 --- a/internal/ftpuser/write.go +++ b/internal/ftpuser/write.go @@ -68,8 +68,11 @@ type Spec struct { // (ftp_path_syntax_incorrect, password_syntax_incorrect, …) is left to // the API and surfaces verbatim through the Caller. func (cl *Client) Add(ctx context.Context, s Spec) (string, error) { - if s.Password == "" || s.Comment == "" { - return "", errors.New("ftpuser: add_ftpuser requires a non-empty password and comment") + switch { + case s.Password == "": + return "", errors.New("ftpuser: add_ftpuser requires a non-empty password") + case s.Comment == "": + return "", errors.New("ftpuser: add_ftpuser requires a non-empty comment") } resp, err := kaswrite.Call(ctx, cl.c, "ftpuser", addAction, AddParams(s)) if err != nil { diff --git a/internal/mailaccount/mailaccount.go b/internal/mailaccount/mailaccount.go index 18b1910..3167ca6 100644 --- a/internal/mailaccount/mailaccount.go +++ b/internal/mailaccount/mailaccount.go @@ -138,17 +138,19 @@ func DecodeMailAccounts(returnInfo soap.Value) (MailAccountList, error) { // TableHeaders returns the columns used by --output=table for // MailAccountList. func (MailAccountList) TableHeaders() []string { - return []string{"LOGIN", "ADDRESS", "USED_MB", "RESPONDER", "ACTIVE", "IN_PROGRESS"} + return []string{"LOGIN", "ADDRESS", "USED", "RESPONDER", "ACTIVE", "IN_PROGRESS"} } -// TableRows emits one row per MailAccount entry. +// TableRows emits one row per MailAccount entry. used_mailaccount_space +// carries the " MB" unit as part of the value to match the singular +// detail row and the cross-module convention. func (l MailAccountList) TableRows() [][]string { rows := make([][]string, 0, len(l)) for _, a := range l { rows = append(rows, []string{ a.Login, a.Addresses, - strconv.FormatFloat(a.UsedSpace, 'f', 2, 64), + strconv.FormatFloat(a.UsedSpace, 'f', 2, 64) + " MB", a.Responder, a.IsActive, a.InProgress, @@ -183,7 +185,7 @@ func (a MailAccount) TableRows() [][]string { {"mail_xlist_trash", a.XListTrash}, {"mail_xlist_spam", a.XListSpam}, {"mail_xlist_archiv", a.XListArchiv}, - {"used_mailaccount_space", strconv.FormatFloat(a.UsedSpace, 'f', 2, 64)}, + {"used_mailaccount_space", strconv.FormatFloat(a.UsedSpace, 'f', 2, 64) + " MB"}, {"mail_is_active", a.IsActive}, {"show_password", a.ShowPassword}, {"mail_allow_nets", a.AllowNets}, diff --git a/internal/mailforward/write.go b/internal/mailforward/write.go index 50e1fc4..6e67432 100644 --- a/internal/mailforward/write.go +++ b/internal/mailforward/write.go @@ -28,10 +28,12 @@ const ( // value is rejected before any SOAP call so the CLI can surface a fast // validation error. func (cl *Client) Add(ctx context.Context, localPart, domainPart string, targets []string) (string, error) { - if localPart == "" || domainPart == "" { - return "", errors.New("mailforward: add_mailforward requires a non-empty local and domain part") - } - if len(targets) == 0 { + switch { + case localPart == "": + return "", errors.New("mailforward: add_mailforward requires a non-empty local part") + case domainPart == "": + return "", errors.New("mailforward: add_mailforward requires a non-empty domain part") + case len(targets) == 0: return "", errors.New("mailforward: add_mailforward requires at least one target") } resp, err := kaswrite.Call(ctx, cl.c, "mailforward", addAction, AddParams(localPart, domainPart, targets)) diff --git a/internal/mailinglist/write.go b/internal/mailinglist/write.go index ff9aad1..3800f84 100644 --- a/internal/mailinglist/write.go +++ b/internal/mailinglist/write.go @@ -43,10 +43,12 @@ const ( // rejected before any SOAP call so the CLI can surface a fast // validation error. func (cl *Client) Add(ctx context.Context, name, domain, password string) (string, error) { - if name == "" || domain == "" { - return "", errors.New("mailinglist: add_mailinglist requires a non-empty name and domain") - } - if password == "" { + switch { + case name == "": + return "", errors.New("mailinglist: add_mailinglist requires a non-empty name") + case domain == "": + return "", errors.New("mailinglist: add_mailinglist requires a non-empty domain") + case password == "": return "", errors.New("mailinglist: add_mailinglist requires a non-empty password") } resp, err := kaswrite.Call(ctx, cl.c, "mailinglist", addAction, AddParams(name, domain, password)) diff --git a/internal/sambauser/write.go b/internal/sambauser/write.go index 0eac4e3..cdc2411 100644 --- a/internal/sambauser/write.go +++ b/internal/sambauser/write.go @@ -62,8 +62,13 @@ type Spec struct { // (path_syntax_incorrect, password_syntax_incorrect, …) is left to the // API and surfaces verbatim through the Caller. func (cl *Client) Add(ctx context.Context, s Spec) (string, error) { - if s.Password == "" || s.Comment == "" || s.Path == "" { - return "", errors.New("sambauser: add_sambauser requires a non-empty password, comment and path") + switch { + case s.Password == "": + return "", errors.New("sambauser: add_sambauser requires a non-empty password") + case s.Comment == "": + return "", errors.New("sambauser: add_sambauser requires a non-empty comment") + case s.Path == "": + return "", errors.New("sambauser: add_sambauser requires a non-empty path") } resp, err := kaswrite.Call(ctx, cl.c, "sambauser", addAction, AddParams(s)) if err != nil {