diff --git a/AGENTS.md b/AGENTS.md index f6b13c3..eee6285 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,6 +85,43 @@ This issues one cheap `GET /realms` call. A `200` reports `available`/`probe_suc Important: `band sip status` **does not persist** its result anywhere. Run it again any time you need a fresh answer, and don't expect `band auth status` to start reporting anything other than `unknown` for a role-holding credential — that command stays fully offline by design. +#### 10DLC capability (tri-state, not boolean) + +`band auth status --plain` reports `tendlc` as `{"status":..., "reason":...}` rather than a +boolean, for the same reason as SIP: access needs both the Campaign Management role and the +account-level Registration Center feature, and only the role is knowable offline. + +- `unavailable` / `role_absent` — the credential lacks the Campaign Management role. +- `unknown` / `role_present_not_probed` — run `band tendlc status` to resolve. + +`campaign_management` remains a plain boolean meaning "the credential holds the role", and +`customer_profiles` is a separate capability — the Customer Profiles Access role is distinct. + +```bash +band tendlc status --plain +# → {"access":"available","mode":{"reason":"not_discoverable","status":"unknown"},"reason":"probe_succeeded"} +``` + +`band tendlc status` can emit any of these `reason` values. Every 403-derived result +exits **0** — the probe answered the question, even when the answer is negative — so +there is no stderr message to fall back on; this table is the only authoritative place +to learn what to do next. `probe_failed` is the sole exception: the probe couldn't +answer at all, so it exits non-zero and is the only reason worth retrying. + +| `reason` | `access` | Exit code | Next action | +|---|---|---|---| +| `probe_succeeded` | `available` | 0 | Registration Center access confirmed. Proceed with `band tendlc` commands. | +| `registration_center_not_enabled` | `unavailable` | 0 | Account doesn't have the Registration Center feature. Escalate to your Bandwidth account manager to enable it — do not retry. | +| `campaign_management_not_enabled` | `unavailable` | 0 | Campaign Management/messaging is not enabled on the account. Escalate to your Bandwidth account manager — do not retry. | +| `role_absent` | `unavailable` | 0 | The credential lacks the Campaign Management role. Have an account manager assign the role — do not retry; a retry hits the same 403. | +| `access_denied` | `unavailable` | 0 | A 403 that didn't match any recognized cause. Escalate to your Bandwidth account manager — do not retry. | +| `probe_failed` | `unknown` | non-zero | The probe itself failed (rate limited, 5xx, or a transport error) — it could not answer the question. This is the only reason where retrying makes sense. | + +**`mode` is always `unknown`, by design.** An account either registers campaigns *directly* or +*imports* them from TCR — never both — and that is a property of the account's Bandwidth setup, +not something the API exposes. Do not infer it, and do not try one path to see what happens. +If direct-vs-import was not given to you in the task, stop and ask the operator. + ### Account Hint When multiple accounts or profiles are active, commands write a hint to stderr so you know which account is being targeted: diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index 962503e..cd995b0 100644 --- a/cmd/auth/auth_test.go +++ b/cmd/auth/auth_test.go @@ -6,7 +6,10 @@ import ( "path/filepath" "testing" + "github.com/spf13/cobra" + "github.com/Bandwidth/cli/internal/config" + "github.com/Bandwidth/cli/internal/testutil" ) func TestCmdStructure(t *testing.T) { @@ -211,6 +214,182 @@ func TestHasRole(t *testing.T) { } } +func TestTenDLCCapability(t *testing.T) { + got := tendlcCapability(true) + if got["status"] != "unknown" || got["reason"] != "role_present_not_probed" { + t.Errorf("tendlcCapability(true) = %v, want unknown/role_present_not_probed", got) + } + got = tendlcCapability(false) + if got["status"] != "unavailable" || got["reason"] != "role_absent" { + t.Errorf("tendlcCapability(false) = %v, want unavailable/role_absent", got) + } +} + +// Customer Profiles Access is a distinct role from campaign_management — +// confirmed on a live credential that carries both. +func TestCustomerProfilesCapabilityIsSeparate(t *testing.T) { + caps := Capabilities([]string{"campaign_management"}) + if caps["customer_profiles"] { + t.Error("customer_profiles should be false without the Customer Profiles Access role") + } + if !caps["campaign_management"] { + t.Error("campaign_management should be true") + } + + caps = Capabilities([]string{"Customer Profiles Access"}) + if !caps["customer_profiles"] { + t.Error("customer_profiles should be true with the role") + } + if caps["campaign_management"] { + t.Error("campaign_management should be false without its own role") + } +} + +// liveAccount9901287Roles is the (trimmed) role list observed on a live +// credential for account 9901287 — captured directly rather than invented, so +// tests exercise the tri-state wiring against a realistic role string, not +// just an idealized one. In particular it confirms the role is genuinely +// snake_case "campaign_management" (not the "Campaign Management" display +// string used elsewhere for error messages), and it includes a decoy — +// "specialized customer external tns" — that contains the word "customer" but +// is not a Customer Profiles Access role. +var liveAccount9901287Roles = []string{ + "Alerting Insights", "Analytics", "Billing Reports", + "campaign_management", "Configuration", "Customer Profiles Access", + "Disconnect", "E911 Management", "specialized customer external tns", + "HTTP Application Management", "HttpVoice", "Line Features", + "Ordering", "Porting", "Reporting", "Short Code Access", +} + +// removeRole returns a copy of roles with every occurrence of target removed. +func removeRole(roles []string, target string) []string { + out := make([]string, 0, len(roles)) + for _, r := range roles { + if r != target { + out = append(out, r) + } + } + return out +} + +// TestTenDLCWiringAgainstLiveRoles exercises hasRole, tendlcCapability, and +// Capabilities together against the real role strings from +// liveAccount9901287Roles, rather than testing tendlcCapability(bool) in +// isolation. TestTenDLCCapability and TestCustomerProfilesCapabilityIsSeparate +// already cover the functions' own logic; this test guards the wiring itself +// — a future change to hasRole's matching (e.g. adding normalization that +// treats "_" and " " differently, or an accidental typo in the substring +// passed at the call site) could silently make campaign_management always +// evaluate to role_absent while every existing test still passes. +func TestTenDLCWiringAgainstLiveRoles(t *testing.T) { + if !hasRole(liveAccount9901287Roles, "campaign_management") { + t.Fatal("hasRole should find campaign_management in the live role list") + } + got := tendlcCapability(hasRole(liveAccount9901287Roles, "campaign_management")) + if got["status"] != "unknown" || got["reason"] != "role_present_not_probed" { + t.Errorf("tendlcCapability with live roles = %v, want unknown/role_present_not_probed", got) + } + if !Capabilities(liveAccount9901287Roles)["customer_profiles"] { + t.Error("customer_profiles should be true with live roles (Customer Profiles Access is present)") + } + + withoutCampaign := removeRole(liveAccount9901287Roles, "campaign_management") + if hasRole(withoutCampaign, "campaign_management") { + t.Fatal("hasRole should not find campaign_management once it's removed from the role list") + } + got = tendlcCapability(hasRole(withoutCampaign, "campaign_management")) + if got["status"] != "unavailable" || got["reason"] != "role_absent" { + t.Errorf("tendlcCapability without campaign_management = %v, want unavailable/role_absent", got) + } + + withoutCustomerProfiles := removeRole(liveAccount9901287Roles, "Customer Profiles Access") + if Capabilities(withoutCustomerProfiles)["customer_profiles"] { + t.Error("customer_profiles should be false once Customer Profiles Access is removed from the role list") + } +} + +// TestStatusPlainTenDLCAgreesWithCapabilities is an end-to-end regression +// test on `band auth status --plain`'s actual JSON output, not just on the +// helper functions in isolation. The bug lived in the call-site wiring +// inside runStatus (status.go), which fed tendlcCapability a *different* +// signal — hasRole(p.Roles, "campaign_management"), a plain substring match +// on the unnormalized snake_case string — than the one behind +// capabilities.campaign_management, which is Contains(rl, "campaign") over +// the same roles. For the display-form role "Campaign Management" those two +// signals disagree: the boolean matches (it contains "campaign") but the old +// hasRole lookup does not (no role contains the literal substring +// "campaign_management" with an underscore). That produced a single JSON +// document asserting both capabilities.campaign_management=true and +// tendlc={"status":"unavailable","reason":"role_absent"} for the same +// credential. Testing tendlcCapability(caps["campaign_management"]) directly +// would trivially pass by construction regardless of what runStatus itself +// does; only driving runStatus end to end (as done here) actually exercises +// the wiring and would have failed before the fix. +func TestStatusPlainTenDLCAgreesWithCapabilities(t *testing.T) { + tests := []struct { + name string + roles []string + }{ + {name: "snake_case role as seen on live account 9901287", roles: []string{"campaign_management"}}, + {name: "display-form role", roles: []string{"Campaign Management"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + + cfgPath, err := config.DefaultPath() + if err != nil { + t.Fatal(err) + } + cfg := &config.Config{Format: "json"} + cfg.SetProfile("default", &config.Profile{ + ClientID: "id1", + AccountID: "ACCT_A", + Roles: tt.roles, + }) + if err := config.Save(cfgPath, cfg); err != nil { + t.Fatal(err) + } + + wrap := &cobra.Command{Use: "status", RunE: runStatus} + root := testutil.NewTestRoot(wrap) + root.SetArgs([]string{"status", "--plain"}) + + out := testutil.CaptureStdout(t, func() { + if err := root.Execute(); err != nil { + t.Fatalf("Execute() error = %v", err) + } + }) + + var got statusJSON + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("unmarshal output: %v\noutput: %s", err, out) + } + + if !got.Capabilities["campaign_management"] { + t.Fatalf("capabilities.campaign_management = false for roles %v, want true", tt.roles) + } + if got.TenDLC["reason"] == "role_absent" { + t.Errorf("capabilities.campaign_management = true but tendlc = %v — the two must agree in one JSON document", got.TenDLC) + } + }) + } +} + +// TestCustomerProfilesMatcherNotOverBroad guards against loosening the +// customer_profiles matcher from "customer profiles" to just "customer". +// "specialized customer external tns" is a real role from +// liveAccount9901287Roles that contains "customer" but has nothing to do with +// Customer Profiles Access — it must not flip customer_profiles to true. +func TestCustomerProfilesMatcherNotOverBroad(t *testing.T) { + caps := Capabilities([]string{"specialized customer external tns"}) + if caps["customer_profiles"] { + t.Error("customer_profiles should be false for 'specialized customer external tns' — it contains 'customer' but is not a Customer Profiles Access role") + } +} + // TestRunSwitch_PersistsTargetIntoActiveProfile guards against the bug where // switch only updated the legacy top-level cfg.AccountID, leaving the active // profile's AccountID stale — so subsequent commands continued targeting the diff --git a/cmd/auth/status.go b/cmd/auth/status.go index a1c5be0..019c918 100644 --- a/cmd/auth/status.go +++ b/cmd/auth/status.go @@ -42,8 +42,11 @@ type statusJSON struct { // SIP reports SIP provisioning availability as a tri-state object // ({"status":..., "reason":...}) rather than a bool inside Capabilities — // see sipCapability. - SIP map[string]string `json:"sip,omitempty"` - Error string `json:"error,omitempty"` + SIP map[string]string `json:"sip,omitempty"` + // TenDLC reports Registration Center availability as a tri-state, for the + // same reason as SIP — see tendlcCapability. + TenDLC map[string]string `json:"tendlc,omitempty"` + Error string `json:"error,omitempty"` } func runStatus(cmd *cobra.Command, args []string) error { @@ -82,6 +85,7 @@ func runStatus(cmd *cobra.Command, args []string) error { _, keychainErr := intauth.GetPassword(p.ClientID) if plain { + caps := Capabilities(p.Roles) out := statusJSON{ Authenticated: keychainErr == nil, Profile: profileName, @@ -91,8 +95,9 @@ func runStatus(cmd *cobra.Command, args []string) error { Environment: env, Build: p.Build, Roles: p.Roles, - Capabilities: Capabilities(p.Roles), + Capabilities: caps, SIP: sipCapability(hasRole(p.Roles, "sip credentials")), + TenDLC: tendlcCapability(caps["campaign_management"]), } if keychainErr != nil { out.Error = "credentials not found in keychain" @@ -123,11 +128,13 @@ func runStatus(cmd *cobra.Command, args []string) error { } else if len(p.Accounts) == 0 && p.AccountID == "" { fmt.Println("Scope: system-wide (use --account-id to target an account)") } + caps := Capabilities(p.Roles) if p.Build { fmt.Printf("Type: %s (voice-only, credit-based)\n", ui.Bold("Bandwidth Build")) - fmt.Printf("Capable of: %s\n", capabilitySummary(Capabilities(p.Roles))) + fmt.Printf("Capable of: %s\n", capabilitySummary(caps)) } fmt.Printf("SIP: %s\n", sipSummary(sipCapability(hasRole(p.Roles, "sip credentials")))) + fmt.Printf("10DLC: %s\n", tendlcSummary(tendlcCapability(caps["campaign_management"]))) if env != "prod" || cfg.HasMultipleEnvironments() { fmt.Printf("Environment: %s\n", env) } @@ -156,6 +163,7 @@ func Capabilities(roles []string) map[string]bool { "numbers": false, "vcp": false, "campaign_management": false, + "customer_profiles": false, "tfv": false, } for _, r := range roles { @@ -178,6 +186,9 @@ func Capabilities(roles []string) map[string]bool { if strings.Contains(rl, "campaign") { caps["campaign_management"] = true } + if strings.Contains(rl, "customer profiles") { + caps["customer_profiles"] = true + } if strings.Contains(rl, "tfv") || strings.Contains(rl, "toll-free") || strings.Contains(rl, "tollfree") { caps["tfv"] = true } @@ -224,6 +235,34 @@ func sipSummary(sip map[string]string) string { } } +// tendlcCapability reports 10DLC Registration Center availability as a +// tri-state. Access needs both the Campaign Management role and the +// account-level Registration Center feature; only the role is knowable +// offline, so a boolean would over-promise. Mirrors sipCapability. +// +// Callers must pass caps["campaign_management"] from the same Capabilities() +// call used for the campaign_management boolean, not a separately-matched +// hasRole lookup — otherwise the two can disagree on a single credential +// (e.g. a display-form role string) even though they describe the same fact. +func tendlcCapability(hasRole bool) map[string]string { + if !hasRole { + return map[string]string{"status": "unavailable", "reason": "role_absent"} + } + return map[string]string{"status": "unknown", "reason": "role_present_not_probed"} +} + +// tendlcSummary renders the offline tri-state for human-readable output. +func tendlcSummary(t map[string]string) string { + switch t["reason"] { + case "role_absent": + return ui.Muted("not available (missing Campaign Management role)") + case "role_present_not_probed": + return ui.Muted("unknown — run 'band tendlc status' to check") + default: + return t["status"] + } +} + // capabilitySummary renders a capability map as a "have / not" line // for the human-readable auth status output on Build accounts. func capabilitySummary(caps map[string]bool) string { diff --git a/cmd/tendlc/status.go b/cmd/tendlc/status.go new file mode 100644 index 0000000..db94c69 --- /dev/null +++ b/cmd/tendlc/status.go @@ -0,0 +1,129 @@ +package tendlc + +import ( + "errors" + "strings" + + "github.com/spf13/cobra" + + "github.com/Bandwidth/cli/internal/api" + "github.com/Bandwidth/cli/internal/cmdutil" + "github.com/Bandwidth/cli/internal/output" + tendlcsvc "github.com/Bandwidth/cli/internal/tendlc" +) + +func init() { Cmd.AddCommand(statusCmd) } + +// service builds a tendlc Service for the active account. It is a package +// var, not a plain func, so tests can substitute a service pointed at a stub +// server — the same seam pattern as cmd/sip's `service` and +// cmdutil.VoiceClient. +var service = func(cmd *cobra.Command) (*tendlcsvc.Service, error) { + client, acctID, err := cmdutil.PlatformClient(cmdutil.AccountIDFlag(cmd)) + if err != nil { + return nil, err + } + return tendlcsvc.NewService(client, acctID), nil +} + +// modeUnknown is returned on every code path. Account mode (direct vs +// import) is a property of how the account is configured with Bandwidth, not +// a runtime-discoverable fact: brand.imported is true on direct AND import +// accounts, and campaign.imported requires an existing campaign plus a +// detail call. Reporting the field as explicitly unknown beats omitting it, +// which invites callers to assume a default. +func modeUnknown() map[string]string { + return map[string]string{"status": "unknown", "reason": "not_discoverable"} +} + +// statusResult maps a probe outcome onto the stable --plain shape. +// +// The 403 branches are checked in a deliberate order — Registration Center, +// then campaign management, then role — because a body could in principle +// contain more than one of these substrings and the first match wins. This +// order reports the most fundamental blocker first: an account-level feature +// gap (Registration Center or campaign management not enabled) is a bigger +// blocker than a missing role on the credential, so it's surfaced ahead of +// role_absent. +func statusResult(statusCode int, body string) map[string]any { + res := map[string]any{"mode": modeUnknown()} + switch { + case statusCode >= 200 && statusCode < 300: + res["access"], res["reason"] = "available", "probe_succeeded" + case statusCode == 403 && strings.Contains(body, "not enabled for the Registration Center"): + res["access"], res["reason"] = "unavailable", "registration_center_not_enabled" + case statusCode == 403 && strings.Contains(body, "is not enabled on account"): + res["access"], res["reason"] = "unavailable", "campaign_management_not_enabled" + case statusCode == 403 && strings.Contains(body, "does not have access rights"): + res["access"], res["reason"] = "unavailable", "role_absent" + case statusCode == 403: + // A 403 we don't recognize is still a definite negative — the probe + // answered the question. Do not guess a specific cause. + res["access"], res["reason"] = "unavailable", "access_denied" + default: + res["access"], res["reason"] = "unknown", "probe_failed" + } + return res +} + +var statusCmd = &cobra.Command{ + Use: "status", + Short: "Check whether this account can use the 10DLC Registration Center", + Long: `Probes the Registration Center API to resolve the "unknown" capability reported +by 'band auth status'. Access requires both the Campaign Management role and the +account-level Registration Center feature; only the probe can confirm the latter. + +Reports access only. Account mode — whether you register campaigns directly or +import them from TCR — is not probed and cannot be discovered: an account is one +or the other, and that is a property of your Bandwidth setup. If you don't know +which yours is, ask your Bandwidth account contact rather than guessing.`, + Example: ` band tendlc status --plain`, + // Contract: probe success is coupled to envelope decoding. svc.ListBrands + // parses the response body before returning, so a 200 whose body is not + // valid JSON (or whose envelope shape has changed) surfaces as an error + // here exactly like a transport failure, and is reported as + // unknown/probe_failed rather than available/probe_succeeded. This is + // deliberate, not a gap to fix: the command cannot distinguish "access is + // fine but the payload changed" from a genuine failure, and reporting + // available off a response it could not actually read would be a false + // positive — worse than a false probe_failed, which just means "run it + // again." + RunE: func(cmd *cobra.Command, args []string) error { + svc, err := service(cmd) + if err != nil { + return err + } + format, plain := cmdutil.OutputFlags(cmd) + + _, probeErr := svc.ListBrands(1, 0, nil) + if probeErr == nil { + return output.StdoutAuto(format, plain, statusResult(200, "")) + } + + // Default to unknown/probe_failed so that EVERY failure path — a + // recognized 403, an unrecognized 403, a 429/5xx, or a bare transport + // error (DNS, connection refused, TLS, timeout, context cancellation) + // — emits a stable JSON document on stdout. A caller parsing stdout + // must never see an empty body just because the failure didn't happen + // to arrive wrapped in *api.APIError. + res := statusResult(0, "") + var apiErr *api.APIError + if errors.As(probeErr, &apiErr) { + res = statusResult(apiErr.StatusCode, apiErr.Body) + // A 403 is a probe that succeeded in answering the question: + // this account cannot use the Registration Center. Exit 0 — the + // command did its job. + if apiErr.StatusCode == 403 { + return output.StdoutAuto(format, plain, res) + } + } + // Anything else — 429, 5xx, or a transport error that never made it + // to an HTTP response — is a failure to answer, not an answer. Emit + // the result for callers parsing stdout, then fall through to the + // normal non-zero error path. + if emitErr := output.StdoutAuto(format, plain, res); emitErr != nil { + return emitErr + } + return roleGateError(probeErr, "Campaign Management") + }, +} diff --git a/cmd/tendlc/status_test.go b/cmd/tendlc/status_test.go new file mode 100644 index 0000000..7c000f9 --- /dev/null +++ b/cmd/tendlc/status_test.go @@ -0,0 +1,225 @@ +package tendlc + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/Bandwidth/cli/internal/api" + tendlcsvc "github.com/Bandwidth/cli/internal/tendlc" + "github.com/Bandwidth/cli/internal/testutil" +) + +// withStubStatusService swaps the package-level `service` var for the +// duration of a test so `band tendlc status` hits a stub server instead of +// performing real auth — the same seam pattern cmd/sip's `service` var uses. +func withStubStatusService(t *testing.T, baseURL string) { + t.Helper() + orig := service + t.Cleanup(func() { service = orig }) + service = func(cmd *cobra.Command) (*tendlcsvc.Service, error) { + return tendlcsvc.NewService(api.NewClientNoAuth(baseURL), "9901287"), nil + } +} + +// statusStubServerWithCode returns a stub that answers GET .../brands (the +// cheap probe `band tendlc status` issues via ListBrands) with the given +// status code and body. +func statusStubServerWithCode(t *testing.T, code int, body string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || !strings.HasSuffix(r.URL.Path, "/brands") { + w.WriteHeader(404) + return + } + w.WriteHeader(code) + _, _ = w.Write([]byte(body)) + })) +} + +// runStatus executes `band tendlc status --plain` against whatever `service` +// currently resolves to, returning stdout and the command's error together — +// the pairing that matters for this command's exit-code contract. +func runStatus(t *testing.T) (string, error) { + t.Helper() + root := testutil.NewTestRoot(statusCmd) + root.SetArgs([]string{"status", "--plain"}) + + var err error + out := testutil.CaptureStdout(t, func() { + err = root.Execute() + }) + return out, err +} + +func decodeStatusOutput(t *testing.T, out string) map[string]interface{} { + t.Helper() + var got map[string]interface{} + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("output is not JSON: %q (%v)", out, err) + } + return got +} + +func assertModeUnknown(t *testing.T, got map[string]interface{}) { + t.Helper() + mode, ok := got["mode"].(map[string]interface{}) + if !ok { + t.Fatalf("mode missing or wrong type: %#v", got["mode"]) + } + if mode["status"] != "unknown" || mode["reason"] != "not_discoverable" { + t.Errorf("mode = %v, want unknown/not_discoverable", mode) + } +} + +// TestTendlcStatus_ProbeSucceeded covers the 200 path end to end: RunE must +// return nil (exit 0) and stdout must carry access=available/probe_succeeded. +func TestTendlcStatus_ProbeSucceeded(t *testing.T) { + srv := statusStubServerWithCode(t, 200, `{"data":[]}`) + defer srv.Close() + withStubStatusService(t, srv.URL) + + out, err := runStatus(t) + if err != nil { + t.Fatalf("Execute() error = %v, want nil", err) + } + + got := decodeStatusOutput(t, out) + if got["access"] != "available" { + t.Errorf("access = %v, want %q", got["access"], "available") + } + if got["reason"] != "probe_succeeded" { + t.Errorf("reason = %v, want %q", got["reason"], "probe_succeeded") + } + assertModeUnknown(t, got) +} + +// TestTendlcStatus_RecognizedForbidden covers a documented 403: this is the +// rule most at risk of regression — a successful probe reporting a negative +// fact must still exit 0, not surface as a command failure. +func TestTendlcStatus_RecognizedForbidden(t *testing.T) { + srv := statusStubServerWithCode(t, 403, + `{"errors":[{"description":"client does not have access rights to the content"}]}`) + defer srv.Close() + withStubStatusService(t, srv.URL) + + out, err := runStatus(t) + if err != nil { + t.Fatalf("Execute() error = %v, want nil (a 403 probe result is not a command failure)", err) + } + + got := decodeStatusOutput(t, out) + if got["access"] != "unavailable" { + t.Errorf("access = %v, want %q", got["access"], "unavailable") + } + if got["reason"] != "role_absent" { + t.Errorf("reason = %v, want %q", got["reason"], "role_absent") + } + assertModeUnknown(t, got) +} + +// TestTendlcStatus_UnrecognizedForbidden covers a 403 body that doesn't match +// any of the three documented substrings: still a successful probe (exit 0), +// mapped to the access_denied fallback reason. +func TestTendlcStatus_UnrecognizedForbidden(t *testing.T) { + srv := statusStubServerWithCode(t, 403, `{"errors":[{"description":"something new"}]}`) + defer srv.Close() + withStubStatusService(t, srv.URL) + + out, err := runStatus(t) + if err != nil { + t.Fatalf("Execute() error = %v, want nil (a 403 probe result is not a command failure)", err) + } + + got := decodeStatusOutput(t, out) + if got["access"] != "unavailable" { + t.Errorf("access = %v, want %q", got["access"], "unavailable") + } + if got["reason"] != "access_denied" { + t.Errorf("reason = %v, want %q", got["reason"], "access_denied") + } + assertModeUnknown(t, got) +} + +// TestTendlcStatus_ServerError covers the probe_failed path for a 5xx: the +// probe's job IS to report Registration Center availability, so even though +// this exits non-zero, an agent branching on stable JSON fields still needs +// unknown/probe_failed on stdout rather than an empty body. +func TestTendlcStatus_ServerError(t *testing.T) { + srv := statusStubServerWithCode(t, 503, `{}`) + defer srv.Close() + withStubStatusService(t, srv.URL) + + out, err := runStatus(t) + if err == nil { + t.Fatal("Execute() error = nil, want a non-nil error for HTTP 503") + } + + got := decodeStatusOutput(t, out) + if got["access"] != "unknown" { + t.Errorf("access = %v, want %q", got["access"], "unknown") + } + if got["reason"] != "probe_failed" { + t.Errorf("reason = %v, want %q", got["reason"], "probe_failed") + } + assertModeUnknown(t, got) +} + +// TestTendlcStatus_MalformedSuccessBody locks in the documented coupling +// between probe success and envelope decoding: svc.ListBrands parses the +// response body before returning, so a 200 whose body is not valid JSON +// cannot be distinguished from a genuine failure. The command must not +// report available/probe_succeeded off a body it could not read — it reports +// unknown/probe_failed and returns a non-nil error, same as any other +// failure to answer. +func TestTendlcStatus_MalformedSuccessBody(t *testing.T) { + srv := statusStubServerWithCode(t, 200, `not json at all`) + defer srv.Close() + withStubStatusService(t, srv.URL) + + out, err := runStatus(t) + if err == nil { + t.Fatal("Execute() error = nil, want a non-nil error for an undecodable 200 body") + } + + got := decodeStatusOutput(t, out) + if got["access"] != "unknown" { + t.Errorf("access = %v, want %q", got["access"], "unknown") + } + if got["reason"] != "probe_failed" { + t.Errorf("reason = %v, want %q", got["reason"], "probe_failed") + } + assertModeUnknown(t, got) +} + +// TestTendlcStatus_TransportFailure is the regression lock for the bug where +// a bare transport error (never wrapped in *api.APIError) produced EMPTY +// stdout: RunE fell straight through to roleGateError without emitting +// anything. Closing the server before the request forces a connection +// refused, which api.Client wraps as a plain error, not *api.APIError. +func TestTendlcStatus_TransportFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + srv.Close() // closed before use: any request now fails at the transport layer + withStubStatusService(t, srv.URL) + + out, err := runStatus(t) + if err == nil { + t.Fatal("Execute() error = nil, want a non-nil error for a transport failure") + } + if strings.TrimSpace(out) == "" { + t.Fatal("stdout is empty; want a stable unknown/probe_failed JSON document even on transport failure") + } + + got := decodeStatusOutput(t, out) + if got["access"] != "unknown" { + t.Errorf("access = %v, want %q", got["access"], "unknown") + } + if got["reason"] != "probe_failed" { + t.Errorf("reason = %v, want %q", got["reason"], "probe_failed") + } + assertModeUnknown(t, got) +} diff --git a/cmd/tendlc/tendlc_test.go b/cmd/tendlc/tendlc_test.go index da7ebf4..505dd19 100644 --- a/cmd/tendlc/tendlc_test.go +++ b/cmd/tendlc/tendlc_test.go @@ -220,6 +220,60 @@ func TestFilterNumbers(t *testing.T) { }) } +func TestStatusCommandRegistered(t *testing.T) { + c, _, err := Cmd.Find([]string{"status"}) + if err != nil || c.Name() != "status" { + t.Fatalf("Find(status) = %v, err %v; want the status command", c, err) + } +} + +func TestStatusResultShape(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + wantAccess string + wantReason string + }{ + {"success", 200, `{"data":[]}`, "available", "probe_succeeded"}, + {"role missing", 403, `{"errors":[{"description":"does not have access rights"}]}`, + "unavailable", "role_absent"}, + {"registration center off", 403, `{"errors":[{"description":"not enabled for the Registration Center"}]}`, + "unavailable", "registration_center_not_enabled"}, + {"campaign mgmt off", 403, `{"errors":[{"description":"10DLC is not enabled on account"}]}`, + "unavailable", "campaign_management_not_enabled"}, + {"unrecognized 403", 403, `{"errors":[{"description":"something new"}]}`, + "unavailable", "access_denied"}, + {"server error", 503, `{}`, "unknown", "probe_failed"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := statusResult(tt.statusCode, tt.body) + if got["access"] != tt.wantAccess { + t.Errorf("access = %v, want %v", got["access"], tt.wantAccess) + } + if got["reason"] != tt.wantReason { + t.Errorf("reason = %v, want %v", got["reason"], tt.wantReason) + } + }) + } +} + +// mode is always present and always unknown — an omitted field invites +// callers to guess a default. +func TestStatusAlwaysReportsModeUnknown(t *testing.T) { + for _, code := range []int{200, 403, 503} { + got := statusResult(code, `{}`) + mode, ok := got["mode"].(map[string]string) + if !ok { + t.Fatalf("code %d: mode missing or wrong type: %#v", code, got["mode"]) + } + if mode["status"] != "unknown" || mode["reason"] != "not_discoverable" { + t.Errorf("code %d: mode = %v, want unknown/not_discoverable", code, mode) + } + } +} + func contains(s, sub string) bool { return len(s) >= len(sub) && (s == sub || len(s) > 0 && containsSubstring(s, sub)) } diff --git a/internal/api/client.go b/internal/api/client.go index db2abfe..3fd8e99 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -1,3 +1,5 @@ +// Package api provides the shared HTTP client, response-envelope parsing, +// and query encoding used by Bandwidth's platform API packages. package api import ( @@ -8,6 +10,7 @@ import ( "mime/multipart" "net/http" "net/textproto" + "strconv" "strings" "time" @@ -26,6 +29,32 @@ func userAgent() string { type APIError struct { StatusCode int Body string + // Header is the response header set. Populated whenever api.Client itself + // constructs the error (the doRaw and PostXMLReturnLocation paths), so + // callers can honor Retry-After and the X-Rate-Limit-* family, which only + // ever appear on the 429 that needs them. Code elsewhere that builds an + // APIError from a parsed response body rather than an *http.Response — + // internal/sip is the current example — may leave this nil. RetryAfter + // returns false in that case; treat that as "unknown," not "none sent." + Header http.Header +} + +// RetryAfter reports the Retry-After delay if the server sent one. Only the +// delta-seconds form is supported — Bandwidth's v2 A2P APIs do not send the +// HTTP-date form. +func (e *APIError) RetryAfter() (time.Duration, bool) { + if e.Header == nil { + return 0, false + } + v := strings.TrimSpace(e.Header.Get("Retry-After")) + if v == "" { + return 0, false + } + secs, err := strconv.Atoi(v) + if err != nil || secs < 0 { + return 0, false + } + return time.Duration(secs) * time.Second, true } func (e *APIError) Error() string { @@ -136,7 +165,7 @@ func (c *Client) doRaw(req *http.Request) ([]byte, error) { } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return nil, &APIError{StatusCode: resp.StatusCode, Body: string(data)} + return nil, &APIError{StatusCode: resp.StatusCode, Body: string(data), Header: resp.Header.Clone()} } return data, nil } @@ -279,7 +308,7 @@ func (c *Client) PostXMLReturnLocation(path string, body XMLBody) (string, error return "", fmt.Errorf("reading response body: %w", err) } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return "", &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} + return "", &APIError{StatusCode: resp.StatusCode, Body: string(respBody), Header: resp.Header.Clone()} } return resp.Header.Get("Location"), nil } diff --git a/internal/api/client_test.go b/internal/api/client_test.go index c500303..5dca980 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -2,10 +2,12 @@ package api import ( "encoding/json" + "errors" "net/http" "net/http/httptest" "strings" "testing" + "time" "github.com/Bandwidth/cli/internal/auth" ) @@ -466,3 +468,36 @@ func TestXMLClient_NonXMLBodyReturnsError(t *testing.T) { t.Errorf("expected error mentioning XMLBody, got: %v", err) } } + +func TestAPIErrorCapturesHeaders(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "7") + w.Header().Set("X-Rate-Limit-Remaining", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"errors":[]}`)) + })) + defer srv.Close() + + c := NewClientNoAuth(srv.URL) + var out any + err := c.Get("/x", &out) + + var apiErr *APIError + if !errors.As(err, &apiErr) { + t.Fatalf("expected *APIError, got %T", err) + } + if got := apiErr.Header.Get("X-Rate-Limit-Remaining"); got != "0" { + t.Errorf("X-Rate-Limit-Remaining = %q, want %q", got, "0") + } + d, ok := apiErr.RetryAfter() + if !ok || d != 7*time.Second { + t.Errorf("RetryAfter() = %v, %v; want 7s, true", d, ok) + } +} + +func TestRetryAfterAbsentReportsFalse(t *testing.T) { + e := &APIError{StatusCode: 500, Header: http.Header{}} + if _, ok := e.RetryAfter(); ok { + t.Error("RetryAfter() reported ok with no header") + } +} diff --git a/internal/api/envelope.go b/internal/api/envelope.go new file mode 100644 index 0000000..9e5f681 --- /dev/null +++ b/internal/api/envelope.go @@ -0,0 +1,96 @@ +package api + +import ( + "encoding/json" + "fmt" +) + +// Page is the pagination block present on every Bandwidth v2 response that +// supports pagination (10DLC A2P registration and customer profiles, among +// others). +type Page struct { + Number int `json:"pageNumber"` + Size int `json:"pageSize"` + TotalElements int `json:"totalElements"` + TotalPages int `json:"totalPages"` +} + +// Truncated reports whether more records exist beyond the ones retrieved so +// far. returnedSoFar must be the CUMULATIVE count of records collected across +// every page fetched up to this point — not the length of the current page. +// Passing a per-page count gives a correct answer on the first page and a +// wrong one on every subsequent page: e.g. on the last of 5 pages of 2 +// records each, a per-page count of 2 against TotalElements 10 would report +// "truncated" even though nothing remains. +// Driven by totalElements rather than by observing a short page, because a +// full page is not evidence of more and a short page is not evidence of none. +func (p *Page) Truncated(returnedSoFar int) bool { + if p == nil { + return false + } + return returnedSoFar < p.TotalElements +} + +// Envelope is the {data, page, errors, links} wrapper the API returns. +// Data is kept as any because its shape varies per endpoint: an object for +// brand detail, an array for lists and history. The published spec says +// array in both cases; production disagrees, so callers state which they +// expect and get an error rather than a silent zero value if wrong. +// +// Responses are returned as map[string]any rather than typed structs: +// production returns fields absent from the published spec, and +// encoding/json silently drops unknown fields when decoding into a struct. +// Requests are typed; responses are lossless. +type Envelope struct { + Data any + Page *Page +} + +// ParseEnvelope decodes a response body into an Envelope. +func ParseEnvelope(body []byte) (*Envelope, error) { + var raw struct { + Data any `json:"data"` + Page *Page `json:"page"` + } + if err := json.Unmarshal(body, &raw); err != nil { + return nil, fmt.Errorf("decoding response envelope: %w", err) + } + return &Envelope{Data: raw.Data, Page: raw.Page}, nil +} + +// List returns data as an array. Errors if the endpoint returned an object, +// or if data was null or absent — genuinely empty lists come back from the +// API as "data":[], so a nil data field is an anomaly, not an empty result, +// and must not be silently treated as one. +func (e *Envelope) List() ([]any, error) { + arr, ok := e.Data.([]any) + if !ok { + return nil, fmt.Errorf("response data must be an array, got %s", dataShape(e.Data)) + } + return arr, nil +} + +// Object returns data as a single resource. Errors if the endpoint returned +// an array, or if data was null or absent. +func (e *Envelope) Object() (map[string]any, error) { + obj, ok := e.Data.(map[string]any) + if !ok { + return nil, fmt.Errorf("response data must be an object, got %s", dataShape(e.Data)) + } + return obj, nil +} + +// dataShape describes the JSON shape of data in terms a CLI user can act on, +// rather than a Go type name. +func dataShape(data any) string { + switch data.(type) { + case nil: + return "null" + case []any: + return "an array" + case map[string]any: + return "an object" + default: + return fmt.Sprintf("%T", data) + } +} diff --git a/internal/api/envelope_test.go b/internal/api/envelope_test.go new file mode 100644 index 0000000..04b1e49 --- /dev/null +++ b/internal/api/envelope_test.go @@ -0,0 +1,112 @@ +package api + +import "testing" + +func TestParseEnvelopeArrayData(t *testing.T) { + body := []byte(`{"data":[{"brandId":"B1"},{"brandId":"B2"}], + "page":{"pageNumber":0,"pageSize":2,"totalElements":10,"totalPages":5}}`) + env, err := ParseEnvelope(body) + if err != nil { + t.Fatalf("ParseEnvelope: %v", err) + } + list, err := env.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(list) != 2 { + t.Errorf("len(list) = %d, want 2", len(list)) + } + if env.Page == nil || env.Page.TotalElements != 10 || env.Page.TotalPages != 5 { + t.Errorf("Page = %+v, want TotalElements 10 / TotalPages 5", env.Page) + } +} + +// brand get returns data as an OBJECT, contradicting the published spec. +// Verified live on 9901287. Prod wins. +func TestParseEnvelopeObjectData(t *testing.T) { + body := []byte(`{"data":{"brandId":"B0IRNU4","universalEin":"US_562242657"}}`) + env, err := ParseEnvelope(body) + if err != nil { + t.Fatalf("ParseEnvelope: %v", err) + } + obj, err := env.Object() + if err != nil { + t.Fatalf("Object: %v", err) + } + if obj["brandId"] != "B0IRNU4" { + t.Errorf("brandId = %v, want B0IRNU4", obj["brandId"]) + } + // Undocumented field must survive — responses are lossless. + if obj["universalEin"] != "US_562242657" { + t.Errorf("universalEin was dropped: %v", obj["universalEin"]) + } +} + +func TestEnvelopeShapeMismatchIsAnError(t *testing.T) { + env, _ := ParseEnvelope([]byte(`{"data":{"brandId":"B1"}}`)) + if _, err := env.List(); err == nil { + t.Error("List() on object data should error, not silently return empty") + } + env2, _ := ParseEnvelope([]byte(`{"data":[]}`)) + if _, err := env2.Object(); err == nil { + t.Error("Object() on array data should error") + } +} + +// Data that is null, or a data key that is entirely absent, must fail +// closed rather than being read as "no results" — genuinely empty lists +// come back from the API as "data":[], so nil is anomalous, not normal. +func TestEnvelopeNilDataIsAnError(t *testing.T) { + env, err := ParseEnvelope([]byte(`{"data":null}`)) + if err != nil { + t.Fatalf("ParseEnvelope: %v", err) + } + if _, err := env.List(); err == nil { + t.Error("List() on explicit null data should error, not report an empty list") + } + + env2, err := ParseEnvelope([]byte(`{}`)) + if err != nil { + t.Fatalf("ParseEnvelope: %v", err) + } + if _, err := env2.List(); err == nil { + t.Error("List() on a body with no data key should error, not report an empty list") + } +} + +func TestPageTruncated(t *testing.T) { + tests := []struct { + name string + page *Page + returnedSoFar int + want bool + }{ + {"more pages remain", &Page{TotalElements: 10, TotalPages: 5, Size: 2}, 2, true}, + {"everything returned", &Page{TotalElements: 2, TotalPages: 1, Size: 50}, 2, false}, + {"empty result", &Page{TotalElements: 0, TotalPages: 0, Size: 50}, 0, false}, + // Cumulative count across all pages walked equals the total: nothing + // left, even though a naive per-page count would have said otherwise + // on earlier pages. + {"cumulative count reaches total on last page", &Page{TotalElements: 10, TotalPages: 5, Size: 2}, 10, false}, + {"nil page is never truncated", nil, 2, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.page.Truncated(tt.returnedSoFar); got != tt.want { + t.Errorf("Truncated(%d) = %v, want %v", tt.returnedSoFar, got, tt.want) + } + }) + } +} + +// Missing page metadata must be detectable so callers can fail closed +// rather than silently emitting a partial list. +func TestParseEnvelopeMissingPageIsNil(t *testing.T) { + env, err := ParseEnvelope([]byte(`{"data":[]}`)) + if err != nil { + t.Fatalf("ParseEnvelope: %v", err) + } + if env.Page != nil { + t.Errorf("Page = %+v, want nil when absent", env.Page) + } +} diff --git a/internal/api/filter.go b/internal/api/filter.go new file mode 100644 index 0000000..fc9bc28 --- /dev/null +++ b/internal/api/filter.go @@ -0,0 +1,64 @@ +package api + +import ( + "net/url" + "strconv" +) + +// Filter operators. The API uses OpenAPI deepObject encoding, so a filter is +// sent as field[op]=value. Sending the bare field=value form is accepted by +// the server and then IGNORED — the result looks successful but is unfiltered. +// +// This is the full set of operators the tendlc and customerprofile deepObject +// filter parameters document (see ~/Developer/api-specs/external/tendlc.yml, +// components.parameters.*Param): string/enum fields support eq and/or +// contains depending on the field, and date fields (createdDate, +// modifiedDate) support gt and lt — NOT gte/lte, which do not appear +// anywhere in the spec. +const ( + OpEq = "eq" + OpContains = "contains" + OpGt = "gt" + OpLt = "lt" +) + +// Filter is one deepObject query filter. +type Filter struct { + Field string + Op string + Value string +} + +// EncodeQuery builds the query string for a list endpoint. Returns "" when +// there is nothing to send. Parameters are sorted so output is deterministic +// and testable. Filters with an empty Value are omitted entirely rather than +// sent blank, which the API treats as a match-nothing filter. +// +// Filters are keyed by Field+Op, so a later filter with the same Field and Op +// overwrites an earlier one. Callers needing multiple constraints on one field +// (e.g. createdDate[gte] and createdDate[lte]) must use distinct Ops. +func EncodeQuery(limit, offset int, filters []Filter) string { + v := url.Values{} + if limit > 0 { + v.Set("limit", strconv.Itoa(limit)) + } + if offset > 0 { + v.Set("offset", strconv.Itoa(offset)) + } + for _, f := range filters { + if f.Field == "" || f.Value == "" { + continue + } + op := f.Op + if op == "" { + op = OpEq + } + v.Set(f.Field+"["+op+"]", f.Value) + } + if len(v) == 0 { + return "" + } + // url.Values.Encode sorts by key, so output is deterministic and the + // exact-string tests above are stable. + return "?" + v.Encode() +} diff --git a/internal/api/filter_test.go b/internal/api/filter_test.go new file mode 100644 index 0000000..f67d8f1 --- /dev/null +++ b/internal/api/filter_test.go @@ -0,0 +1,74 @@ +package api + +import "testing" + +func TestEncodeQueryDeepObjectForm(t *testing.T) { + tests := []struct { + name string + limit int + offset int + filters []Filter + want string + }{ + { + name: "no params", want: "", + }, + { + name: "limit and offset only", limit: 50, offset: 100, + want: "?limit=50&offset=100", + }, + { + name: "eq filter uses bracket form", + limit: 10, + filters: []Filter{{Field: "status", Op: OpEq, Value: "REGISTERED"}}, + want: "?limit=10&status%5Beq%5D=REGISTERED", + }, + { + name: "contains filter", + filters: []Filter{{Field: "brandId", Op: OpContains, Value: "B0I"}}, + want: "?brandId%5Bcontains%5D=B0I", + }, + { + name: "filters sort by field for determinism", + limit: 5, + filters: []Filter{ + {Field: "vettingStatus", Op: OpEq, Value: "APPROVED"}, + {Field: "brandId", Op: OpEq, Value: "B53K4I0"}, + }, + want: "?brandId%5Beq%5D=B53K4I0&limit=5&vettingStatus%5Beq%5D=APPROVED", + }, + { + name: "empty value is omitted, not sent blank", + filters: []Filter{{Field: "status", Op: OpEq, Value: ""}}, + want: "", + }, + { + name: "value is URL-escaped", + filters: []Filter{{Field: "campaignName", Op: OpContains, Value: "Acme Corp&Co"}}, + want: "?campaignName%5Bcontains%5D=Acme+Corp%26Co", + }, + { + name: "duplicate field+op: last value wins", + filters: []Filter{ + {Field: "status", Op: OpEq, Value: "PENDING"}, + {Field: "status", Op: OpEq, Value: "APPROVED"}, + }, + want: "?status%5Beq%5D=APPROVED", + }, + { + name: "same field, different ops: both kept", + filters: []Filter{ + {Field: "createdDate", Op: OpGt, Value: "2024-01-01"}, + {Field: "createdDate", Op: OpLt, Value: "2024-12-31"}, + }, + want: "?createdDate%5Bgt%5D=2024-01-01&createdDate%5Blt%5D=2024-12-31", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := EncodeQuery(tt.limit, tt.offset, tt.filters); got != tt.want { + t.Errorf("EncodeQuery() =\n %q\nwant\n %q", got, tt.want) + } + }) + } +} diff --git a/internal/cmdutil/exitcodes.go b/internal/cmdutil/exitcodes.go index 7efb370..d4baf7a 100644 --- a/internal/cmdutil/exitcodes.go +++ b/internal/cmdutil/exitcodes.go @@ -2,6 +2,8 @@ package cmdutil import ( "errors" + "sort" + "strings" "github.com/Bandwidth/cli/internal/api" ) @@ -27,6 +29,37 @@ type SecretUnavailableError struct{ Message string } func (e *SecretUnavailableError) Error() string { return e.Message } +// FlagError reports invalid or missing command-line flags. It maps to +// ExitFlagError (6) so agents can distinguish "you called this wrong" — +// fixable without any API state — from a server-side failure. No HTTP +// request has been made when this is returned. +type FlagError struct{ Message string } + +func (e *FlagError) Error() string { return e.Message } + +// NewFlagError returns a FlagError with the given message. +func NewFlagError(msg string) error { return &FlagError{Message: msg} } + +// NewMissingFlagsError reports every missing required flag in one error, +// sorted for determinism. Cobra's MarkFlagRequired is not used on commands +// with conditional requirements, so aggregation happens here instead. +// +// A nil or empty names is a caller bug — there is no missing flag to report — +// but this must still produce a sensible message rather than the malformed +// "missing required flags: " (trailing separator, no names) that naive +// joining would emit. +func NewMissingFlagsError(names []string) error { + if len(names) == 0 { + return &FlagError{Message: "missing required flags"} + } + sorted := append([]string(nil), names...) + sort.Strings(sorted) + for i, n := range sorted { + sorted[i] = "--" + n + } + return &FlagError{Message: "missing required flags: " + strings.Join(sorted, ", ")} +} + // ConflictError reports that the target resource exists but is not in a state // where the requested operation can succeed — a duplicate, a wrong lifecycle // state, a mismatched existing setting. It maps to ExitConflict (4). @@ -47,6 +80,9 @@ func (e *ConflictError) Error() string { return e.Message } func (e *ConflictError) Unwrap() error { return e.Cause } // ExitCodeForError maps an error to the appropriate exit code. +// FlagError takes precedence over everything else, including a wrapped API +// error: it is a client-side validation failure and no HTTP request was ever +// made, so it must win regardless of what else the error chain contains. // FeatureLimitError takes precedence over the raw API status code so a // 403 caused by a plan/role limit maps to ExitConflict (4) rather than // ExitAuth (2) — agents can then distinguish "stop, escalate" from @@ -60,6 +96,10 @@ func ExitCodeForError(err error) int { if errors.Is(err, ErrPollTimeout) { return ExitTimeout } + var flagErr *FlagError + if errors.As(err, &flagErr) { + return ExitFlagError + } var fle *FeatureLimitError if errors.As(err, &fle) { return ExitConflict diff --git a/internal/cmdutil/exitcodes_test.go b/internal/cmdutil/exitcodes_test.go index 4bcaef7..ac93f22 100644 --- a/internal/cmdutil/exitcodes_test.go +++ b/internal/cmdutil/exitcodes_test.go @@ -114,3 +114,46 @@ func TestConflictError_PreservesCauseChain(t *testing.T) { t.Errorf("unwrapped Code = %q, want 33006", got.Code) } } + +func TestFlagErrorExitCode(t *testing.T) { + err := cmdutil.NewFlagError("bad value for --evp") + if got := cmdutil.ExitCodeForError(err); got != cmdutil.ExitFlagError { + t.Errorf("ExitCodeForError = %d, want %d", got, cmdutil.ExitFlagError) + } +} + +func TestMissingFlagsErrorListsAllNames(t *testing.T) { + err := cmdutil.NewMissingFlagsError([]string{"usecase", "description", "sample1"}) + want := "missing required flags: --description, --sample1, --usecase" + if err.Error() != want { + t.Errorf("Error() = %q, want %q", err.Error(), want) + } + if got := cmdutil.ExitCodeForError(err); got != cmdutil.ExitFlagError { + t.Errorf("ExitCodeForError = %d, want %d", got, cmdutil.ExitFlagError) + } +} + +// An empty or nil names slice must not produce the malformed +// "missing required flags: " (trailing separator, no names). Locks the +// guard added for this case. +func TestMissingFlagsErrorEmptyNames(t *testing.T) { + want := "missing required flags" + if got := cmdutil.NewMissingFlagsError(nil).Error(); got != want { + t.Errorf("NewMissingFlagsError(nil).Error() = %q, want %q", got, want) + } + if got := cmdutil.NewMissingFlagsError([]string{}).Error(); got != want { + t.Errorf("NewMissingFlagsError([]string{}).Error() = %q, want %q", got, want) + } +} + +// A FlagError must win over a wrapped APIError: it is a client-side +// failure and no request was ever sent. +func TestFlagErrorTakesPrecedenceOverAPIError(t *testing.T) { + // Build an error chain with both FlagError and APIError; FlagError must win. + // If the flagErr branch in ExitCodeForError is moved after APIError handling, + // this test fails: the 403 APIError (ExitAuth=2) would be checked first. + wrapped := fmt.Errorf("flag problem: %w (during %w)", cmdutil.NewFlagError("bad"), &api.APIError{StatusCode: 403, Body: "forbidden"}) + if got := cmdutil.ExitCodeForError(wrapped); got != cmdutil.ExitFlagError { + t.Errorf("ExitCodeForError = %d, want %d", got, cmdutil.ExitFlagError) + } +} diff --git a/internal/cmdutil/poll.go b/internal/cmdutil/poll.go index 37e7f79..78ba7ef 100644 --- a/internal/cmdutil/poll.go +++ b/internal/cmdutil/poll.go @@ -1,6 +1,7 @@ package cmdutil import ( + "context" "errors" "fmt" "time" @@ -13,6 +14,9 @@ var ErrPollTimeout = errors.New("operation did not complete in time") // PollConfig configures a polling loop. type PollConfig struct { + // Context cancels the poll loop. Optional — nil means context.Background(). + // Existing callers omit it and keep the previous behavior exactly. + Context context.Context Interval time.Duration Timeout time.Duration // Check performs one poll attempt. It should return done=true when the @@ -25,7 +29,12 @@ type PollConfig struct { // cfg.Timeout is exceeded. On success it returns the result from Check. // On timeout it returns ErrPollTimeout. func Poll(cfg PollConfig) (interface{}, error) { + ctx := cfg.Context + if ctx == nil { + ctx = context.Background() + } deadline := time.Now().Add(cfg.Timeout) + for { done, result, err := cfg.Check() if err != nil { @@ -37,6 +46,19 @@ func Poll(cfg PollConfig) (interface{}, error) { if time.Now().After(deadline) { return nil, fmt.Errorf("timed out after %s: %w", cfg.Timeout, ErrPollTimeout) } - time.Sleep(cfg.Interval) + // A fresh timer per iteration. Go 1.23+ made timer channels + // unbuffered and Stop() cancels any in-flight send, so there is + // nothing left to drain after Stop() returns — draining an + // already-stopped timer's channel would just block forever. + // Stop() on the cancellation path is a courtesy; an abandoned + // timer is garbage collected once unreferenced, so there is no + // leak either way. + timer := time.NewTimer(cfg.Interval) + select { + case <-ctx.Done(): + timer.Stop() + return nil, ctx.Err() + case <-timer.C: + } } } diff --git a/internal/cmdutil/poll_test.go b/internal/cmdutil/poll_test.go index 3ac1906..1a79f28 100644 --- a/internal/cmdutil/poll_test.go +++ b/internal/cmdutil/poll_test.go @@ -1,6 +1,7 @@ package cmdutil import ( + "context" "errors" "testing" "time" @@ -48,3 +49,42 @@ func TestPollPropagatesCheckError(t *testing.T) { t.Fatalf("expected boom, got %v", err) } } + +func TestPollRespectsContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + calls := 0 + go func() { time.Sleep(30 * time.Millisecond); cancel() }() + + _, err := Poll(PollConfig{ + Context: ctx, + Interval: 10 * time.Millisecond, + Timeout: 10 * time.Second, + Check: func() (bool, interface{}, error) { + calls++ + return false, nil, nil + }, + }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("err = %v, want context.Canceled", err) + } + // Poll always calls Check at least once before it can observe + // cancellation, so calls == 0 could never fail here. Assert instead + // that the loop actually iterated more than once — the interval + // (10ms) is well under the 30ms cancellation delay, so a working + // loop calls Check several times before ctx.Done() is observed. + if calls < 2 { + t.Errorf("calls = %d, want > 1 (loop should iterate before cancellation)", calls) + } +} + +// A nil Context must behave exactly as before — all existing callers omit it. +func TestPollNilContextStillTimesOut(t *testing.T) { + _, err := Poll(PollConfig{ + Interval: 5 * time.Millisecond, + Timeout: 20 * time.Millisecond, + Check: func() (bool, interface{}, error) { return false, nil, nil }, + }) + if !errors.Is(err, ErrPollTimeout) { + t.Fatalf("err = %v, want ErrPollTimeout", err) + } +} diff --git a/internal/customerprofile/service.go b/internal/customerprofile/service.go new file mode 100644 index 0000000..3d40cec --- /dev/null +++ b/internal/customerprofile/service.go @@ -0,0 +1,53 @@ +// Package customerprofile wraps Bandwidth's Numbers v2 customer-profile API. +// +// Customer profiles are a prerequisite for 10DLC brand registration, and a +// profile backs EXACTLY ONE brand — reusing one fails with "cannot be +// assigned to another brand". This 1:1 constraint is undocumented in the +// published specs and was found by probing production. +package customerprofile + +import ( + "fmt" + "net/url" + + "github.com/Bandwidth/cli/internal/api" +) + +// Service wraps the customer-profile endpoints for one account. +type Service struct { + client *api.Client + accountID string +} + +// NewService returns a Service bound to an account. c must be a JSON client +// (see cmdutil.PlatformClient). +func NewService(c *api.Client, accountID string) *Service { + return &Service{client: c, accountID: accountID} +} + +func (s *Service) base() string { + return "/api/v2/accounts/" + url.PathEscape(s.accountID) + "/customerProfiles" +} + +func (s *Service) get(path string) (*api.Envelope, error) { + raw, err := s.client.GetRaw(path) + if err != nil { + return nil, err + } + return api.ParseEnvelope(raw) +} + +// List returns customer profiles on the account. +func (s *Service) List(limit, offset int, filters []api.Filter) (*api.Envelope, error) { + return s.get(s.base() + api.EncodeQuery(limit, offset, filters)) +} + +// Get returns one customer profile. Soft-deleted profiles are still +// returned individually, with a "deleted" flag set — check it before +// creating any association. +func (s *Service) Get(profileID string) (*api.Envelope, error) { + if profileID == "" { + return nil, fmt.Errorf("customer profile ID is required") + } + return s.get(s.base() + "/" + url.PathEscape(profileID)) +} diff --git a/internal/customerprofile/service_test.go b/internal/customerprofile/service_test.go new file mode 100644 index 0000000..82e83b1 --- /dev/null +++ b/internal/customerprofile/service_test.go @@ -0,0 +1,149 @@ +package customerprofile + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/Bandwidth/cli/internal/api" +) + +func newTestService(t *testing.T, h http.HandlerFunc) (*Service, func()) { + t.Helper() + return newTestServiceForAccount(t, "9901287", h) +} + +func newTestServiceForAccount(t *testing.T, accountID string, h http.HandlerFunc) (*Service, func()) { + t.Helper() + srv := httptest.NewServer(h) + return NewService(api.NewClientNoAuth(srv.URL), accountID), srv.Close +} + +func TestListBuildsPathAndQuery(t *testing.T) { + var gotPath, gotQuery string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotPath, gotQuery = r.URL.Path, r.URL.RawQuery + _, _ = w.Write([]byte(`{"data":[],"page":{"totalElements":0}}`)) + }) + defer done() + + _, err := svc.List(10, 0, nil) + if err != nil { + t.Fatalf("List: %v", err) + } + if want := "/api/v2/accounts/9901287/customerProfiles"; gotPath != want { + t.Errorf("path = %q, want %q", gotPath, want) + } + if want := "limit=10"; gotQuery != want { + t.Errorf("query = %q, want %q", gotQuery, want) + } +} + +func TestListWithFilterBuildsQuery(t *testing.T) { + var gotQuery string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotQuery = r.URL.RawQuery + _, _ = w.Write([]byte(`{"data":[],"page":{"totalElements":0}}`)) + }) + defer done() + + _, err := svc.List(25, 5, []api.Filter{{Field: "brandId", Op: api.OpEq, Value: "B0IRNU4"}}) + if err != nil { + t.Fatalf("List: %v", err) + } + if want := "brandId%5Beq%5D=B0IRNU4&limit=25&offset=5"; gotQuery != want { + t.Errorf("query = %q, want %q", gotQuery, want) + } +} + +func TestListEscapesAccountID(t *testing.T) { + var gotPath string + svc, done := newTestServiceForAccount(t, "99/../../etc", func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.EscapedPath() + _, _ = w.Write([]byte(`{"data":[]}`)) + }) + defer done() + + _, err := svc.List(0, 0, nil) + if err != nil { + t.Fatalf("List: %v", err) + } + if want := "/api/v2/accounts/99%2F..%2F..%2Fetc/customerProfiles"; gotPath != want { + t.Errorf("escaped path = %q, want %q", gotPath, want) + } +} + +func TestGetReturnsObjectEnvelope(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":{"customerProfileId":"CP123"}}`)) + }) + defer done() + + env, err := svc.Get("CP123") + if err != nil { + t.Fatalf("Get: %v", err) + } + obj, err := env.Object() + if err != nil { + t.Fatalf("Object: %v", err) + } + if obj["customerProfileId"] != "CP123" { + t.Errorf("customerProfileId = %v", obj["customerProfileId"]) + } +} + +func TestGetEscapesID(t *testing.T) { + var gotPath string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.EscapedPath() + _, _ = w.Write([]byte(`{"data":{}}`)) + }) + defer done() + + _, _ = svc.Get("CP/../evil") + if want := "/api/v2/accounts/9901287/customerProfiles/CP%2F..%2Fevil"; gotPath != want { + t.Errorf("escaped path = %q, want %q", gotPath, want) + } +} + +// TestGetEmptyIDErrorsBeforeRequest asserts not just that Get("") errors, but +// that it does so WITHOUT making a request — the handler calls t.Fatal if +// hit, so this fails if the empty-ID guard is ever removed or short-circuited +// after the request is issued. +func TestGetEmptyIDErrorsBeforeRequest(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + t.Fatal("server should not be called for an empty profile ID") + }) + defer done() + + if _, err := svc.Get(""); err == nil { + t.Fatal("expected an error for an empty profile ID") + } +} + +func TestServicePropagatesAPIError(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"errors":[{"description":"does not have access rights"}]}`)) + }) + defer done() + + _, err := svc.List(0, 0, nil) + if err == nil { + t.Fatal("expected an error for 403") + } + + // Callers (e.g. band customer-profile status) need to branch on the HTTP + // status code, which requires *api.APIError to survive this layer + // unwrapped rather than being flattened into a plain error string. Assert + // the type and the status, not just "an error happened" — this fails if + // the service wraps with fmt.Errorf("%v", err) instead of "%w". + var apiErr *api.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error type = %T, want *api.APIError (or something wrapping it)", err) + } + if apiErr.StatusCode != http.StatusForbidden { + t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, http.StatusForbidden) + } +} diff --git a/internal/tendlc/service.go b/internal/tendlc/service.go new file mode 100644 index 0000000..49fc2d9 --- /dev/null +++ b/internal/tendlc/service.go @@ -0,0 +1,64 @@ +// Package tendlc wraps Bandwidth's v2 A2P Campaign Management (10DLC +// Registration Center) API. +package tendlc + +import ( + "fmt" + "net/url" + + "github.com/Bandwidth/cli/internal/api" +) + +// Service wraps the v2 A2P Campaign Management endpoints for one account. +type Service struct { + client *api.Client + accountID string +} + +// NewService returns a Service bound to an account. c must be a JSON client +// (see cmdutil.PlatformClient). +func NewService(c *api.Client, accountID string) *Service { + return &Service{client: c, accountID: accountID} +} + +func (s *Service) base() string { + return "/api/v2/accounts/" + url.PathEscape(s.accountID) + "/tendlc" +} + +// get issues a GET and parses the standard {data, page} envelope. +func (s *Service) get(path string) (*api.Envelope, error) { + raw, err := s.client.GetRaw(path) + if err != nil { + return nil, err + } + return api.ParseEnvelope(raw) +} + +// ListBrands returns the brands on the account. +func (s *Service) ListBrands(limit, offset int, filters []api.Filter) (*api.Envelope, error) { + return s.get(s.base() + "/brands" + api.EncodeQuery(limit, offset, filters)) +} + +// GetBrand returns one brand. The response wraps data as an object, not a +// single-element array — verified against production. +func (s *Service) GetBrand(brandID string) (*api.Envelope, error) { + if brandID == "" { + return nil, fmt.Errorf("brand ID is required") + } + return s.get(s.base() + "/brands/" + url.PathEscape(brandID)) +} + +// ListCampaigns returns the campaigns on the account. The list projection +// omits fields the campaign schema defines (imported, cspId, samples, +// messageFlow) — use GetCampaign when those are needed. +func (s *Service) ListCampaigns(limit, offset int, filters []api.Filter) (*api.Envelope, error) { + return s.get(s.base() + "/campaigns" + api.EncodeQuery(limit, offset, filters)) +} + +// GetCampaign returns one campaign, including the fields the list omits. +func (s *Service) GetCampaign(campaignID string) (*api.Envelope, error) { + if campaignID == "" { + return nil, fmt.Errorf("campaign ID is required") + } + return s.get(s.base() + "/campaigns/" + url.PathEscape(campaignID)) +} diff --git a/internal/tendlc/service_test.go b/internal/tendlc/service_test.go new file mode 100644 index 0000000..16b5c92 --- /dev/null +++ b/internal/tendlc/service_test.go @@ -0,0 +1,183 @@ +package tendlc + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/Bandwidth/cli/internal/api" +) + +func newTestService(t *testing.T, h http.HandlerFunc) (*Service, func()) { + t.Helper() + return newTestServiceForAccount(t, "9901287", h) +} + +func newTestServiceForAccount(t *testing.T, accountID string, h http.HandlerFunc) (*Service, func()) { + t.Helper() + srv := httptest.NewServer(h) + return NewService(api.NewClientNoAuth(srv.URL), accountID), srv.Close +} + +func TestListBrandsBuildsPathAndQuery(t *testing.T) { + var gotPath, gotQuery string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotPath, gotQuery = r.URL.Path, r.URL.RawQuery + _, _ = w.Write([]byte(`{"data":[],"page":{"totalElements":0}}`)) + }) + defer done() + + _, err := svc.ListBrands(25, 0, []api.Filter{{Field: "brandType", Op: api.OpEq, Value: "PUBLIC_PROFIT"}}) + if err != nil { + t.Fatalf("ListBrands: %v", err) + } + if want := "/api/v2/accounts/9901287/tendlc/brands"; gotPath != want { + t.Errorf("path = %q, want %q", gotPath, want) + } + if want := "brandType%5Beq%5D=PUBLIC_PROFIT&limit=25"; gotQuery != want { + t.Errorf("query = %q, want %q", gotQuery, want) + } +} + +func TestGetBrandReturnsObjectEnvelope(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":{"brandId":"B0IRNU4"}}`)) + }) + defer done() + + env, err := svc.GetBrand("B0IRNU4") + if err != nil { + t.Fatalf("GetBrand: %v", err) + } + obj, err := env.Object() + if err != nil { + t.Fatalf("Object: %v", err) + } + if obj["brandId"] != "B0IRNU4" { + t.Errorf("brandId = %v", obj["brandId"]) + } +} + +func TestGetBrandEscapesID(t *testing.T) { + var gotPath string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.EscapedPath() + _, _ = w.Write([]byte(`{"data":{}}`)) + }) + defer done() + + _, _ = svc.GetBrand("B/../evil") + if want := "/api/v2/accounts/9901287/tendlc/brands/B%2F..%2Fevil"; gotPath != want { + t.Errorf("escaped path = %q, want %q", gotPath, want) + } +} + +func TestListBrandsEscapesAccountID(t *testing.T) { + var gotPath string + svc, done := newTestServiceForAccount(t, "99/../../etc", func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.EscapedPath() + _, _ = w.Write([]byte(`{"data":[]}`)) + }) + defer done() + + _, err := svc.ListBrands(0, 0, nil) + if err != nil { + t.Fatalf("ListBrands: %v", err) + } + if want := "/api/v2/accounts/99%2F..%2F..%2Fetc/tendlc/brands"; gotPath != want { + t.Errorf("escaped path = %q, want %q", gotPath, want) + } +} + +func TestServicePropagatesAPIError(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"errors":[{"description":"does not have access rights"}]}`)) + }) + defer done() + + _, err := svc.ListBrands(0, 0, nil) + if err == nil { + t.Fatal("expected an error for 403") + } + + // band tendlc status (Task 9) distinguishes "no Registration Center + // access" (403, a definite answer) from a transport failure by doing + // errors.As(err, &apiErr) and branching on apiErr.StatusCode. That + // contract only holds if *api.APIError survives this layer unwrapped + // and untyped-away — assert both, not just "an error happened". + var apiErr *api.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error type = %T, want *api.APIError (or something wrapping it)", err) + } + if apiErr.StatusCode != http.StatusForbidden { + t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, http.StatusForbidden) + } +} + +func TestListCampaignsBuildsPathAndQuery(t *testing.T) { + var gotPath, gotQuery string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotPath, gotQuery = r.URL.Path, r.URL.RawQuery + _, _ = w.Write([]byte(`{"data":[],"page":{"totalElements":0}}`)) + }) + defer done() + + _, err := svc.ListCampaigns(10, 5, nil) + if err != nil { + t.Fatalf("ListCampaigns: %v", err) + } + if want := "/api/v2/accounts/9901287/tendlc/campaigns"; gotPath != want { + t.Errorf("path = %q, want %q", gotPath, want) + } + if want := "limit=10&offset=5"; gotQuery != want { + t.Errorf("query = %q, want %q", gotQuery, want) + } +} + +func TestGetCampaignEscapesID(t *testing.T) { + var gotPath string + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.EscapedPath() + _, _ = w.Write([]byte(`{"data":{"campaignId":"C123"}}`)) + }) + defer done() + + env, err := svc.GetCampaign("C/../evil") + if err != nil { + t.Fatalf("GetCampaign: %v", err) + } + if want := "/api/v2/accounts/9901287/tendlc/campaigns/C%2F..%2Fevil"; gotPath != want { + t.Errorf("escaped path = %q, want %q", gotPath, want) + } + obj, err := env.Object() + if err != nil { + t.Fatalf("Object: %v", err) + } + if obj["campaignId"] != "C123" { + t.Errorf("campaignId = %v", obj["campaignId"]) + } +} + +func TestGetBrandEmptyIDErrors(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + t.Fatal("server should not be called for an empty brand ID") + }) + defer done() + + if _, err := svc.GetBrand(""); err == nil { + t.Fatal("expected an error for an empty brand ID") + } +} + +func TestGetCampaignEmptyIDErrors(t *testing.T) { + svc, done := newTestService(t, func(w http.ResponseWriter, r *http.Request) { + t.Fatal("server should not be called for an empty campaign ID") + }) + defer done() + + if _, err := svc.GetCampaign(""); err == nil { + t.Fatal("expected an error for an empty campaign ID") + } +}