diff --git a/AGENTS.md b/AGENTS.md index 98d7308..ffd17c5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,6 +36,28 @@ band auth switch # change active account (no re-auth needed) band auth status # verify auth state ``` +### Verified authentication status + +`band auth status --plain` now performs a fresh OAuth token exchange by default. `authenticated: true` means this verification succeeded; a cached token is not sufficient because a client secret may have been revoked after the token was issued. Verification refreshes reported roles, capabilities, Build type, and accessible accounts from the returned JWT without modifying saved configuration or the selected account. It verifies credentials, not authorization for every API operation or the selected account. + +The existing JSON fields are retained, with two additions: `credentials_stored` (client ID and a non-empty secret are available from the configured keychain) and `token`: + +| Situation | `authenticated` | `token.status` | `token.reason` | Exit | +|---|---|---|---|---| +| Verified | true | valid | omitted; `expires_in` gives remaining seconds | 0 | +| Token endpoint rejects credentials (401, or 400 `invalid_client`) | false | rejected | recognized OAuth code, or `token_endpoint_error` | 2 | +| Network, malformed response/JWT, environment, or server failure | false | unknown | probe_failed | 1 | +| Token endpoint rate limit | false | unknown | probe_failed | 7 | +| No client ID | false | unknown | not_logged_in | 2 | +| Keychain secret missing/unavailable/empty | false | unknown | credentials_unavailable | 2 | +| `--no-verify`, credentials available | false | unknown | not_verified | 0 | + +**Migration:** previously `authenticated` only meant the keychain lookup succeeded and status was offline. For that presence check, use `band auth status --no-verify --plain` and inspect `credentials_stored`. Offline mode reports stored metadata, never calls the token endpoint or background update checker, and exits 0 even when credentials are absent (with the corresponding missing-credential reason above). Config/output errors still fail. Automation that requires usable credentials should use default verification and gate on `authenticated` or `token.status == "valid"`; an unknown result is not evidence of rejection. + +Structured status results go to stdout before verification failures return nonzero; human-readable status and remediation go to stderr. Token values, client secrets, and raw OAuth error bodies are never printed. Ordinary API commands also classify token-exchange credential rejection as exit 2, with a profile-specific login command; token 429 remains exit 7 and token 5xx remains exit 1. Existing resource-API 403 mappings are unchanged. Runtime errors omit usage text; parse/argument/required-flag errors and typed `FlagError` validation errors retain it. + +Token exchange honors command cancellation and its 15-second HTTP timeout. Verification uses the same environment selection as API commands (`--environment` > `BW_ENVIRONMENT` > profile; `BW_API_URL` overrides the endpoint) and does not require an account ID. SIP and 10DLC account-level availability still require their own probes below. + ### Credential Profiles Store multiple credential sets under named profiles — useful when different roles or environments require different client credentials: @@ -70,12 +92,12 @@ SIP provisioning (`band sip realm ...`, `band sip credential ...`) needs **two** | `reason` | `status` | Meaning | |----------|----------|---------| | `role_absent` | `unavailable` | Credential lacks the `SIP Credentials` role. | -| `role_present_not_probed` | `unknown` | Credential has the role, but `auth status` is offline and cannot confirm account-level configuration. | +| `role_present_not_probed` | `unknown` | Credential has the role, but `auth status` does not probe SIP account-level configuration. | | `account_not_enabled` | `unavailable` | Only returned by `band sip status` — the account has the role but SIP Credentials isn't enabled on the account. Contact Bandwidth support. | | `probe_succeeded` | `available` | Only returned by `band sip status` — the account can use SIP provisioning. | | `probe_failed` | `unknown` | Only returned by `band sip status` — the probe itself failed (e.g. rate limited or a server error); retry later. | -`band auth status` never calls the network, so it can only ever report `role_absent` or `role_present_not_probed` for `sip`. To resolve an `unknown`, run the explicit probe: +`band auth status` verifies the credentials, but does not probe SIP account settings, so it can only report `role_absent` or `role_present_not_probed` for `sip`. To resolve an `unknown`, run the explicit probe: ```bash band sip status --plain @@ -83,7 +105,7 @@ band sip status --plain This issues one cheap `GET /realms` call. A `200` reports `available`/`probe_succeeded` (exit 0). Hitting error code `33004` ("account isn't setup for Sip Credentials") reports `unavailable`/`account_not_enabled` — and **exits 0**, because a successful probe that confirms a negative fact is not a command failure. Auth errors (401/403) exit 2 via the normal error path; rate limiting or server errors exit non-zero with `probe_failed`. -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. +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 — credential verification does not confirm SIP account settings. #### 10DLC capability (tri-state, not boolean) diff --git a/README.md b/README.md index f8ada9f..d5e7cd0 100644 --- a/README.md +++ b/README.md @@ -378,7 +378,8 @@ Sub-accounts (formerly known as sites) are the top-level container. Locations (f |---------|-------------| | `band auth login` | Log in with OAuth2 credentials (use `--profile ` to store under a named profile) | | `band auth logout` | Clear stored credentials | -| `band auth status` | Show auth state, active account, and accessible accounts | +| `band auth status` | Verify credentials with a fresh token exchange; show auth state, active account, and accessible accounts | +| `band auth status --no-verify` | Inspect stored credentials offline; does not assert authentication | | `band auth switch [id]` | Switch to a different account | | `band auth profiles` | List all stored credential profiles | | `band auth use ` | Switch the active credential profile | @@ -615,7 +616,7 @@ All five share the same filters: `--to`/`--from` (comma-separated E.164), `--dir **"account ID not set"** — You're logged in but haven't picked an account. Run `band auth switch ` or pass `--account-id`. -**"credential verification failed"** — Your client ID or secret is wrong. Double-check them in the Bandwidth App. +**"credentials were rejected"** — The token endpoint rejected your client ID or secret (exit 2). Follow the profile-specific `band auth login --profile ...` instruction. Network errors and token-endpoint failures are reported separately; they do not prove that credentials are invalid. **API error 401** — Your token expired. Run `band auth login` again. diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index cd995b0..a9dd88c 100644 --- a/cmd/auth/auth_test.go +++ b/cmd/auth/auth_test.go @@ -326,6 +326,9 @@ func TestTenDLCWiringAgainstLiveRoles(t *testing.T) { // 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) { + originalPassword := statusPassword + statusPassword = func(string) (string, error) { return "test-secret", nil } + t.Cleanup(func() { statusPassword = originalPassword }) tests := []struct { name string roles []string @@ -354,6 +357,7 @@ func TestStatusPlainTenDLCAgreesWithCapabilities(t *testing.T) { } wrap := &cobra.Command{Use: "status", RunE: runStatus} + wrap.Flags().Bool("no-verify", true, "") root := testutil.NewTestRoot(wrap) root.SetArgs([]string{"status", "--plain"}) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 087d99a..f659e37 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -126,7 +126,8 @@ func runLogin(cmd *cobra.Command, args []string) error { spin := ui.NewSpinner("Verifying credentials...") spin.Start() tm := intauth.NewTokenManager(clientID, clientSecret, tokenURL) - token, err := tm.GetToken() + tm.ProfileName = profileName + token, err := tm.GetTokenContext(cmd.Context()) spin.Stop() if err != nil { return fmt.Errorf("credential verification failed: %w", err) diff --git a/cmd/auth/status.go b/cmd/auth/status.go index 019c918..a0ac924 100644 --- a/cmd/auth/status.go +++ b/cmd/auth/status.go @@ -2,8 +2,8 @@ package auth import ( "encoding/json" + "errors" "fmt" - "os" "strings" "github.com/spf13/cobra" @@ -16,29 +16,34 @@ import ( func init() { Cmd.AddCommand(statusCmd) + statusCmd.Flags().Bool("no-verify", false, "Inspect stored credentials without contacting the token endpoint (authenticated remains false)") } var statusCmd = &cobra.Command{ Use: "status", Short: "Show current authentication status", - Long: "Shows the active profile's authentication status, including client ID, active account, environment, and (for Bandwidth Build accounts) capabilities. Use --plain for machine-readable JSON.", + Long: "Verifies the active profile's credentials with a fresh token exchange. Use --no-verify for offline inspection, or --plain for machine-readable JSON. SIP and 10DLC account-level availability still require their separate status probes.", Example: ` band auth status band auth status --plain`, RunE: runStatus, + Args: cobra.NoArgs, } // statusJSON is the structured output shape returned when --plain is set. -// Stable contract for agents — additive changes only. +// Existing fields are retained; authenticated now requires verification. +// Offline consumers migrate to credentials_stored (see AGENTS.md). type statusJSON struct { - Authenticated bool `json:"authenticated"` - Profile string `json:"profile,omitempty"` - ClientID string `json:"client_id,omitempty"` - AccountID string `json:"account_id,omitempty"` - Accounts []string `json:"accounts,omitempty"` - Environment string `json:"environment,omitempty"` - Build bool `json:"build,omitempty"` - Roles []string `json:"roles,omitempty"` - Capabilities map[string]bool `json:"capabilities,omitempty"` + Authenticated bool `json:"authenticated"` + CredentialsStored bool `json:"credentials_stored"` + Token tokenStatus `json:"token"` + Profile string `json:"profile,omitempty"` + ClientID string `json:"client_id,omitempty"` + AccountID string `json:"account_id,omitempty"` + Accounts []string `json:"accounts,omitempty"` + Environment string `json:"environment,omitempty"` + Build bool `json:"build,omitempty"` + Roles []string `json:"roles,omitempty"` + Capabilities map[string]bool `json:"capabilities,omitempty"` // SIP reports SIP provisioning availability as a tri-state object // ({"status":..., "reason":...}) rather than a bool inside Capabilities — // see sipCapability. @@ -49,6 +54,15 @@ type statusJSON struct { Error string `json:"error,omitempty"` } +type tokenStatus struct { + Status string `json:"status"` + Reason string `json:"reason,omitempty"` + ExpiresIn *int `json:"expires_in,omitempty"` +} + +// Seamed for tests so no real OS keychain is read. +var statusPassword = intauth.GetPassword + func runStatus(cmd *cobra.Command, args []string) error { _, plain := cmdutil.OutputFlags(cmd) @@ -64,14 +78,6 @@ func runStatus(cmd *cobra.Command, args []string) error { p := cfg.ActiveProfileConfig() - if p.ClientID == "" { - if plain { - return emitJSON(statusJSON{Authenticated: false}) - } - fmt.Fprintln(os.Stderr, ui.Warn("Not logged in.")) - return nil - } - env := p.Environment if env == "" { env = "prod" @@ -82,74 +88,92 @@ func runStatus(cmd *cobra.Command, args []string) error { profileName = "default" } - _, keychainErr := intauth.GetPassword(p.ClientID) - - if plain { - caps := Capabilities(p.Roles) - out := statusJSON{ - Authenticated: keychainErr == nil, - Profile: profileName, - ClientID: p.ClientID, - AccountID: p.AccountID, - Accounts: p.Accounts, - Environment: env, - Build: p.Build, - Roles: 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" - } - return emitJSON(out) - } - - if keychainErr != nil { - fmt.Printf("Client ID: %s\n", ui.ID(p.ClientID)) - fmt.Printf("Account: %s\n", ui.ID(p.AccountID)) - // Show environment only when it's informative. - if env != "prod" || cfg.HasMultipleEnvironments() { - fmt.Printf("Environment: %s\n", env) - } - fmt.Println("Status: " + ui.Error("credentials not found in keychain")) - return nil + noVerify, _ := cmd.Flags().GetBool("no-verify") + caps := Capabilities(p.Roles) + out := statusJSON{ + Profile: profileName, + ClientID: p.ClientID, + AccountID: p.AccountID, + Accounts: p.Accounts, + Environment: env, + Build: p.Build, + Roles: p.Roles, + Capabilities: caps, + SIP: sipCapability(hasRole(p.Roles, "sip credentials")), + TenDLC: tendlcCapability(caps["campaign_management"]), } - - fmt.Printf("Profile: %s\n", ui.Bold(profileName)) - fmt.Printf("Client ID: %s\n", ui.ID(p.ClientID)) - if p.AccountID != "" { - fmt.Printf("Account: %s\n", ui.ID(p.AccountID)) + out.Token = tokenStatus{Status: "unknown", Reason: "not_verified"} + var verifyErr error + var secret string + if p.ClientID == "" { + out.Token.Reason = "not_logged_in" + verifyErr = &intauth.CredentialError{Reason: out.Token.Reason, Profile: profileName} } else { - fmt.Printf("Account: (none — pass --account-id on commands)\n") + secret, err = statusPassword(p.ClientID) + out.CredentialsStored = err == nil && secret != "" + if !out.CredentialsStored { + out.Token.Reason = "credentials_unavailable" + verifyErr = &intauth.CredentialError{Reason: out.Token.Reason, Profile: profileName} + } } - if len(p.Accounts) > 1 { - fmt.Printf("Accounts: %s\n", strings.Join(p.Accounts, ", ")) - } else if len(p.Accounts) == 0 && p.AccountID == "" { - fmt.Println("Scope: system-wide (use --account-id to target an account)") + if !noVerify && verifyErr == nil { + var tm *intauth.TokenManager + tm, out.Environment, verifyErr = cmdutil.AuthTokenManager(p, secret, profileName) + if verifyErr == nil { + var token string + var expires int + token, expires, verifyErr = tm.Verify(cmd.Context()) + if verifyErr == nil { + var claims *jwtClaims + claims, verifyErr = parseJWTClaims(token) + if verifyErr == nil { + out.Authenticated = true + out.Token = tokenStatus{Status: "valid", ExpiresIn: &expires} + out.Accounts, out.Roles, out.Build = claims.Accounts, claims.Roles, claims.Build + out.Capabilities = Capabilities(claims.Roles) + out.SIP = sipCapability(hasRole(claims.Roles, "sip credentials")) + out.TenDLC = tendlcCapability(out.Capabilities["campaign_management"]) + } + } + } + if verifyErr != nil { + out.Token = tokenStatus{Status: "unknown", Reason: "probe_failed"} + var tokenErr *intauth.TokenError + if errors.As(verifyErr, &tokenErr) && tokenErr.Rejected() { + out.Token = tokenStatus{Status: "rejected", Reason: tokenErr.Code} + } + } } - 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(caps)) + if verifyErr != nil { + out.Error = verifyErr.Error() } - 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) + if plain { + enc := json.NewEncoder(cmd.OutOrStdout()) + enc.SetIndent("", " ") + if err := enc.Encode(out); err != nil { + return err + } + } else { + w := cmd.ErrOrStderr() + fmt.Fprintf(w, "Profile: %s\nClient ID: %s\nAccount: %s\nEnvironment: %s\n", out.Profile, out.ClientID, out.AccountID, out.Environment) + fmt.Fprintf(w, "Status: %s", out.Token.Status) + if out.Token.Reason != "" { + fmt.Fprintf(w, " (%s)", out.Token.Reason) + } + fmt.Fprintln(w) + fmt.Fprintf(w, "Accounts: %s\n", strings.Join(out.Accounts, ", ")) + if out.Build { + fmt.Fprintln(w, "Type: Bandwidth Build (voice-only, credit-based)") + } + fmt.Fprintf(w, "Capable of: %s\nSIP: %s\n10DLC: %s\n", capabilitySummary(out.Capabilities), sipSummary(out.SIP), tendlcSummary(out.TenDLC)) + if len(cfg.Profiles) > 1 { + fmt.Fprintf(w, "Profiles: %s\n", strings.Join(cfg.ProfileNames(), ", ")) + } } - fmt.Println("Status: " + ui.Success("authenticated")) - - if len(cfg.Profiles) > 1 { - fmt.Printf("Profiles: %s\n", strings.Join(cfg.ProfileNames(), ", ")) + if noVerify { + return nil } - return nil -} - -func emitJSON(v statusJSON) error { - enc := json.NewEncoder(os.Stdout) - enc.SetIndent("", " ") - return enc.Encode(v) + return verifyErr } // Capabilities maps a set of JWT role strings to a stable feature map. diff --git a/cmd/auth/status_verification_test.go b/cmd/auth/status_verification_test.go new file mode 100644 index 0000000..e01bdd9 --- /dev/null +++ b/cmd/auth/status_verification_test.go @@ -0,0 +1,156 @@ +package auth + +import ( + "bytes" + "context" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/Bandwidth/cli/internal/cmdutil" + "github.com/Bandwidth/cli/internal/config" + "github.com/Bandwidth/cli/internal/testutil" + "github.com/spf13/cobra" +) + +func TestStatusVerification(t *testing.T) { + claims := base64.RawURLEncoding.EncodeToString([]byte(`{"accounts":["fresh-account"],"roles":["Campaign Management","SIP Credentials"],"express":true}`)) + jwt := "header." + claims + ".signature" + for _, tc := range []struct { + name string + status int + body string + offline bool + missing string + cancel bool + wantStatus string + wantReason string + wantExit int + }{ + {"valid", 200, `{"access_token":"` + jwt + `","expires_in":3600}`, false, "", false, "valid", "", 0}, + {"rejected", 401, `{"error":"invalid_client","error_description":"secret-value"}`, false, "", false, "rejected", "invalid_client", 2}, + {"OAuth 400 rejection", 400, `{"error":"invalid_client"}`, false, "", false, "rejected", "invalid_client", 2}, + {"rate limited", 429, `{}`, false, "", false, "unknown", "probe_failed", 7}, + {"server error", 500, `{}`, false, "", false, "unknown", "probe_failed", 1}, + {"forbidden is not credential rejection", 403, `{}`, false, "", false, "unknown", "probe_failed", 1}, + {"malformed token response", 200, `proxy`, false, "", false, "unknown", "probe_failed", 1}, + {"bad claims", 200, `{"access_token":"not-a-jwt"}`, false, "", false, "unknown", "probe_failed", 1}, + {"offline", 0, "", true, "", false, "unknown", "not_verified", 0}, + {"not logged in", 0, "", false, "id", false, "unknown", "not_logged_in", 2}, + {"keychain unavailable", 0, "", false, "secret", false, "unknown", "credentials_unavailable", 2}, + {"offline missing secret", 0, "", true, "secret", false, "unknown", "credentials_unavailable", 0}, + {"cancelled", 0, "", false, "", true, "unknown", "probe_failed", 1}, + {"network failure", -1, "", false, "", false, "unknown", "probe_failed", 1}, + } { + t.Run(tc.name, func(t *testing.T) { + taskHome := t.TempDir() + t.Setenv("HOME", taskHome) + t.Setenv("USERPROFILE", taskHome) + for _, name := range []string{"BW_CLIENT_ID", "BW_ACCOUNT_ID", "BW_ENVIRONMENT"} { + t.Setenv(name, "") + } + oldOverride := cmdutil.EnvironmentOverride + cmdutil.EnvironmentOverride = "" + t.Cleanup(func() { cmdutil.EnvironmentOverride = oldOverride }) + cfg := &config.Config{} + id := "test-id" + if tc.missing == "id" { + id = "" + } + cfg.SetProfile("admin", &config.Profile{ClientID: id, Roles: []string{"stale-role"}}) + path, err := config.DefaultPath() + if err != nil { + t.Fatal(err) + } + if err := config.Save(path, cfg); err != nil { + t.Fatal(err) + } + before, _ := os.ReadFile(path) + oldPassword := statusPassword + statusPassword = func(string) (string, error) { + if tc.missing == "secret" { + return "", errors.New("keychain unavailable") + } + return "secret-value", nil + } + t.Cleanup(func() { statusPassword = oldPassword }) + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + if tc.status == 0 { + t.Error("unexpected network request") + w.WriteHeader(500) + return + } + if r.URL.Path != "/api/v1/oauth2/token" { + t.Errorf("path = %s", r.URL.Path) + } + w.WriteHeader(tc.status) + fmt.Fprint(w, tc.body) + })) + defer srv.Close() + if tc.status == -1 { + srv.Close() + } + t.Setenv("BW_API_URL", srv.URL) + child := &cobra.Command{Use: "status", RunE: runStatus} + child.Flags().Bool("no-verify", false, "") + root := testutil.NewTestRoot(child) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + args := []string{"status", "--plain"} + if tc.offline { + args = append(args, "--no-verify") + } + root.SetArgs(args) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if tc.cancel { + cancel() + } + err = root.ExecuteContext(ctx) + if code := cmdutil.ExitCodeForError(err); code != tc.wantExit { + t.Fatalf("exit %d, want %d: %v", code, tc.wantExit, err) + } + var out statusJSON + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("invalid JSON: %v: %s", err, stdout.String()) + } + if out.Token.Status != tc.wantStatus || out.Token.Reason != tc.wantReason { + t.Fatalf("token = %+v", out.Token) + } + if out.Authenticated != (tc.wantStatus == "valid") { + t.Fatalf("authenticated = %v", out.Authenticated) + } + if out.CredentialsStored != (tc.missing == "") { + t.Fatalf("credentials_stored = %v", out.CredentialsStored) + } + if strings.Contains(stdout.String()+stderr.String(), "secret-value") || strings.Contains(stdout.String(), jwt) { + t.Fatal("secret leaked") + } + if out.Authenticated { + if out.Token.ExpiresIn == nil || *out.Token.ExpiresIn < 3590 || !out.Capabilities["campaign_management"] || !out.Build || out.Accounts[0] != "fresh-account" { + t.Fatalf("stale/incomplete verification output: %+v", out) + } + } + wantCalls := 1 + if tc.offline || tc.missing != "" || tc.cancel || tc.status == -1 { + wantCalls = 0 + } + if calls != wantCalls { + t.Errorf("calls = %d, want %d", calls, wantCalls) + } + after, _ := os.ReadFile(path) + if !bytes.Equal(before, after) { + t.Fatal("status mutated stored config") + } + }) + } +} diff --git a/cmd/execute.go b/cmd/execute.go new file mode 100644 index 0000000..9a04520 --- /dev/null +++ b/cmd/execute.go @@ -0,0 +1,65 @@ +package cmd + +import ( + "context" + "errors" + + "github.com/Bandwidth/cli/internal/auth" + "github.com/Bandwidth/cli/internal/cmdutil" + "github.com/spf13/cobra" +) + +// executeCommand owns error rendering so runtime failures do not print usage. +// Track entry into RunE, because Cobra validates required flags after pre-runs. +func executeCommand(ctx context.Context, root *cobra.Command) error { + started := false + var restores []func() + var wrap func(*cobra.Command) + wrap = func(c *cobra.Command) { + if run := c.RunE; run != nil { + c.RunE = func(cmd *cobra.Command, args []string) error { + started = true + return run(cmd, args) + } + restores = append(restores, func() { c.RunE = run }) + } + for _, child := range c.Commands() { + wrap(child) + } + } + wrap(root) + silentErrors, silentUsage := root.SilenceErrors, root.SilenceUsage + root.SilenceErrors, root.SilenceUsage = true, true + defer func() { + root.SilenceErrors, root.SilenceUsage = silentErrors, silentUsage + for _, restore := range restores { + restore() + } + }() + command, err := root.ExecuteContextC(ctx) + if err == nil { + return nil + } + if command == nil { + command = root + } + if !silentErrors && (command == root || !command.SilenceErrors) { + // OAuth failures can acquire several API wrappers. Render the actionable + // underlying error while returning the original chain for exit mapping. + display := err + var tokenErr *auth.TokenError + var credentialErr *auth.CredentialError + if errors.As(err, &tokenErr) { + display = tokenErr + } + if errors.As(err, &credentialErr) { + display = credentialErr + } + command.PrintErrln("Error:", display) + } + var flagErr *cmdutil.FlagError + if !silentUsage && (!started || errors.As(err, &flagErr)) { + command.PrintErr(command.UsageString()) + } + return err +} diff --git a/cmd/execute_test.go b/cmd/execute_test.go new file mode 100644 index 0000000..8ed9d0d --- /dev/null +++ b/cmd/execute_test.go @@ -0,0 +1,111 @@ +package cmd + +import ( + "bytes" + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/Bandwidth/cli/internal/api" + "github.com/Bandwidth/cli/internal/auth" + "github.com/Bandwidth/cli/internal/cmdutil" + "github.com/spf13/cobra" +) + +func TestExecuteOAuthFailure(t *testing.T) { + for _, tc := range []struct { + status int + body string + wantExit int + }{ + {401, `{"error":"invalid_client","error_description":"never-print-secret"}`, 2}, + {400, `{"error":"invalid_client"}`, 2}, + {400, `{"error":"invalid_request"}`, 1}, + {429, `{}`, 7}, + {500, `{"error":"invalid_client"}`, 1}, + } { + t.Run(fmt.Sprint(tc.status, tc.wantExit), func(t *testing.T) { + apiCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/oauth2/token" { + apiCalls++ + t.Error("API called despite token failure") + } + w.WriteHeader(tc.status) + fmt.Fprint(w, tc.body) + })) + defer srv.Close() + tm := auth.NewTokenManager("id", "never-print-secret", srv.URL) + tm.ProfileName = "admin" + client := api.NewClient(srv.URL, tm) + root := &cobra.Command{Use: "band"} + root.AddCommand(&cobra.Command{Use: "probe", RunE: func(c *cobra.Command, args []string) error { + var result any + return client.Get(c.Context(), "/resource", &result) + }}) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"probe"}) + err := executeCommand(context.Background(), root) + if got := cmdutil.ExitCodeForError(err); got != tc.wantExit { + t.Fatalf("exit = %d: %v", got, err) + } + if apiCalls != 0 || stdout.Len() != 0 { + t.Fatalf("API calls or unexpected stdout: %s", stdout.String()) + } + if strings.Contains(stderr.String(), "Usage:") || strings.Contains(stderr.String(), "never-print-secret") || strings.Contains(stderr.String(), "obtaining auth token") { + t.Fatalf("unexpected stderr: %s", stderr.String()) + } + if !strings.Contains(stderr.String(), "Error:") { + t.Fatal("missing error message") + } + if tc.wantExit == 2 && !strings.Contains(stderr.String(), "band auth login --profile admin") { + t.Fatalf("missing remediation: %s", stderr.String()) + } + }) + } +} + +func TestExecuteUsageAndRepeatedInvocations(t *testing.T) { + root := &cobra.Command{Use: "band"} + child := &cobra.Command{Use: "probe", Args: cobra.NoArgs, RunE: func(c *cobra.Command, args []string) error { + if c.Flags().Changed("invalid") { + return cmdutil.NewFlagError("invalid field") + } + return fmt.Errorf("runtime failure") + }} + child.Flags().String("required", "", "required value") + child.Flags().Bool("invalid", false, "") + _ = child.MarkFlagRequired("required") + root.AddCommand(child) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + for _, tc := range []struct { + args []string + wantUsage bool + }{ + {[]string{"probe"}, true}, + {[]string{"probe", "--required", "x"}, false}, + {[]string{"probe", "--unknown"}, true}, + {[]string{"probe", "extra"}, true}, + {[]string{"probe", "--invalid"}, true}, + } { + stdout.Reset() + stderr.Reset() + root.SetArgs(tc.args) + if err := executeCommand(context.Background(), root); err == nil { + t.Fatal("expected failure") + } + if got := strings.Contains(stderr.String(), "Usage:"); got != tc.wantUsage { + t.Fatalf("args %v: stderr %s", tc.args, stderr.String()) + } + if root.SilenceUsage || root.SilenceErrors { + t.Fatal("rendering state leaked") + } + } +} diff --git a/cmd/root.go b/cmd/root.go index 8ed0bbb..2931256 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -67,10 +67,15 @@ var rootCmd = &cobra.Command{ cmdutil.EnvironmentOverride = environment // Kick off version check in background so it doesn't slow down the command. - updateResult = make(chan *versionpkg.CheckResult, 1) - go func() { - updateResult <- versionpkg.Check(version) - }() + noVerify, _ := cmd.Flags().GetBool("no-verify") + if cmd.Name() == "status" && cmd.Parent() == authcmd.Cmd && noVerify { + updateResult = nil // Offline auth inspection must not check for updates. + } else { + updateResult = make(chan *versionpkg.CheckResult, 1) + go func() { + updateResult <- versionpkg.Check(version) + }() + } if !term.IsTerminal(int(os.Stdout.Fd())) { // Auto-enable plain mode for non-terminal output (scripts, pipes) // unless the user explicitly chose a different format. @@ -146,7 +151,7 @@ func Execute() error { api.Version = version ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() - return rootCmd.ExecuteContext(ctx) + return executeCommand(ctx, rootCmd) } func GetFormat() string { diff --git a/cmd/sip/status.go b/cmd/sip/status.go index f094153..85b0a64 100644 --- a/cmd/sip/status.go +++ b/cmd/sip/status.go @@ -17,7 +17,7 @@ var statusCmd = &cobra.Command{ Short: "Check whether this account can use SIP provisioning", Long: "Probes the SIP API to resolve the 'unknown' capability reported by 'band auth status'. " + "SIP provisioning requires both the SIP Credentials role and account-level configuration; " + - "only the probe can confirm the latter. The result is not cached — 'band auth status' stays offline.", + "only the probe can confirm the latter. The result is not cached — 'band auth status' verifies credentials but does not probe SIP account settings.", Example: ` band sip status --plain`, RunE: func(cmd *cobra.Command, args []string) error { svc, err := service(cmd) diff --git a/go.mod b/go.mod index 98f3f50..c783042 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/Bandwidth/cli go 1.26.6 require ( + github.com/alessio/shellescape v1.4.1 github.com/briandowns/spinner v1.23.2 github.com/fatih/color v1.19.0 github.com/olekukonko/tablewriter v0.0.5 @@ -13,7 +14,6 @@ require ( ) require ( - github.com/alessio/shellescape v1.4.1 // indirect github.com/cpuguy83/go-md2man/v2 v2.0.4 // indirect github.com/danieljoos/wincred v1.2.0 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect diff --git a/internal/api/client.go b/internal/api/client.go index 0e26c10..047c1a2 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -148,7 +148,7 @@ func (c *Client) newRequest(ctx context.Context, method, path string, body io.Re if c.basicUser != "" { req.SetBasicAuth(c.basicUser, c.basicPassword) } else if c.tm != nil { - token, err := c.tm.GetToken() + token, err := c.tm.GetTokenContext(ctx) if err != nil { return nil, fmt.Errorf("obtaining auth token: %w", err) } diff --git a/internal/auth/errors.go b/internal/auth/errors.go new file mode 100644 index 0000000..68610af --- /dev/null +++ b/internal/auth/errors.go @@ -0,0 +1,43 @@ +package auth + +import ( + "fmt" + + "github.com/alessio/shellescape" +) + +// CredentialError means no usable stored credentials were available. +type CredentialError struct { + Reason string + Profile string +} + +func (e *CredentialError) Error() string { + return fmt.Sprintf("credentials unavailable for profile %q (%s).\nRun: %s", e.Profile, e.Reason, loginCommand(e.Profile)) +} + +// TokenError retains only the HTTP status and a recognized OAuth error code. +// Never retain the response body: proxies and providers may echo secrets. +type TokenError struct { + StatusCode int + Code string + Profile string +} + +func (e *TokenError) Rejected() bool { + return e.StatusCode == 401 || (e.StatusCode == 400 && e.Code == "invalid_client") +} + +func (e *TokenError) Error() string { + if e.Rejected() { + return fmt.Sprintf("credentials were rejected for profile %q (%s); the client ID or secret is invalid or has been revoked.\nRun: %s", e.Profile, e.Code, loginCommand(e.Profile)) + } + return fmt.Sprintf("token exchange failed (HTTP %d, %s) — check connectivity, environment, and BW_API_URL; retry when the token endpoint is available", e.StatusCode, e.Code) +} + +func loginCommand(profile string) string { + if profile == "" { + profile = "default" + } + return "band auth login --profile " + shellescape.Quote(profile) +} diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index 113b7e2..098cb57 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -2,6 +2,7 @@ package auth import ( "bytes" + "context" "encoding/json" "fmt" "io" @@ -17,6 +18,7 @@ type TokenManager struct { ClientID string ClientSecret string TokenURL string + ProfileName string token string expiresAt time.Time @@ -29,6 +31,7 @@ func NewTokenManager(clientID, clientSecret, tokenURL string) *TokenManager { ClientID: clientID, ClientSecret: clientSecret, TokenURL: tokenURL, + ProfileName: "default", } } @@ -42,23 +45,43 @@ type tokenResponse struct { // GetToken returns a valid Bearer token, fetching a new one if the cached token // is missing or within 1 minute of expiry. func (tm *TokenManager) GetToken() (string, error) { + return tm.GetTokenContext(context.Background()) +} + +// GetTokenContext is the cancellable cached-token path used by API requests. +func (tm *TokenManager) GetTokenContext(ctx context.Context) (string, error) { tm.mu.Lock() defer tm.mu.Unlock() + if err := ctx.Err(); err != nil { + return "", err + } // Return cached token if it has more than 1 minute remaining. if tm.token != "" && time.Now().Add(time.Minute).Before(tm.expiresAt) { return tm.token, nil } - return tm.fetchToken() + return tm.fetchToken(ctx) +} + +// Verify always exchanges the credentials, even when a token is cached. +// A cached token can outlive the secret that minted it. +func (tm *TokenManager) Verify(ctx context.Context) (string, int, error) { + tm.mu.Lock() + defer tm.mu.Unlock() + token, err := tm.fetchToken(ctx) + if err != nil { + return "", 0, err + } + return token, max(0, int(time.Until(tm.expiresAt).Seconds())), nil } // fetchToken performs the token exchange. Caller must hold tm.mu. -func (tm *TokenManager) fetchToken() (string, error) { +func (tm *TokenManager) fetchToken(ctx context.Context) (string, error) { form := url.Values{} form.Set("grant_type", "client_credentials") - req, err := http.NewRequest(http.MethodPost, tm.TokenURL+"/api/v1/oauth2/token", strings.NewReader(form.Encode())) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, tm.TokenURL+"/api/v1/oauth2/token", strings.NewReader(form.Encode())) if err != nil { return "", fmt.Errorf("creating token request: %w", err) } @@ -80,7 +103,16 @@ func (tm *TokenManager) fetchToken() (string, error) { } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return "", fmt.Errorf("token exchange failed (HTTP %d): %s", resp.StatusCode, string(body)) + var failure struct { + Code string `json:"error"` + } + _ = json.Unmarshal(body, &failure) + switch failure.Code { + case "invalid_client", "invalid_request", "invalid_grant", "unauthorized_client", "unsupported_grant_type", "invalid_scope", "server_error", "temporarily_unavailable": + default: + failure.Code = "token_endpoint_error" + } + return "", &TokenError{StatusCode: resp.StatusCode, Code: failure.Code, Profile: tm.ProfileName} } // A 2xx whose body isn't a JSON object means we reached something other than diff --git a/internal/auth/verification_test.go b/internal/auth/verification_test.go new file mode 100644 index 0000000..d7f1758 --- /dev/null +++ b/internal/auth/verification_test.go @@ -0,0 +1,80 @@ +package auth + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestTokenVerificationRejectsRevokedSecretDespiteCachedToken(t *testing.T) { + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + if calls == 1 { + fmt.Fprint(w, `{"access_token":"cached-token","expires_in":3600}`) + return + } + w.WriteHeader(401) + fmt.Fprint(w, `{"error":"invalid_client","error_description":"secret-must-not-leak"}`) + })) + defer srv.Close() + tm := NewTokenManager("id", "secret-must-not-leak", srv.URL) + tm.ProfileName = "admin" + if _, err := tm.GetToken(); err != nil { + t.Fatal(err) + } + _, _, err := tm.Verify(context.Background()) + var tokenErr *TokenError + if !errors.As(err, &tokenErr) || !tokenErr.Rejected() || tokenErr.Code != "invalid_client" { + t.Fatalf("expected rejected credentials, got %v", err) + } + if calls != 2 { + t.Fatalf("requests = %d, want fresh verification", calls) + } + if strings.Contains(err.Error(), "secret-must-not-leak") || !strings.Contains(err.Error(), "band auth login --profile admin") { + t.Fatalf("unsafe or unactionable error: %v", err) + } +} + +func TestTokenContextCancelsExchange(t *testing.T) { + started := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + close(started) + <-r.Context().Done() + })) + defer srv.Close() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + tm := NewTokenManager("id", "secret", srv.URL) + done := make(chan error, 1) + go func() { _, err := tm.GetTokenContext(ctx); done <- err }() + <-started + cancel() + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Fatalf("error = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("token exchange did not cancel") + } +} + +func TestTokenErrorDropsUnknownServerText(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) + fmt.Fprint(w, `{"error":"secret-must-not-leak","error_description":"secret-must-not-leak"}`) + })) + defer srv.Close() + _, err := NewTokenManager("id", "secret", srv.URL).GetToken() + if err == nil || strings.Contains(err.Error(), "secret-must-not-leak") { + t.Fatalf("error = %v", err) + } +} diff --git a/internal/cmdutil/auth_verification_test.go b/internal/cmdutil/auth_verification_test.go new file mode 100644 index 0000000..7a34b0a --- /dev/null +++ b/internal/cmdutil/auth_verification_test.go @@ -0,0 +1,47 @@ +package cmdutil + +import ( + "testing" + + "github.com/Bandwidth/cli/internal/auth" + "github.com/Bandwidth/cli/internal/config" +) + +func TestAuthTokenManagerEnvironment(t *testing.T) { + for _, tc := range []struct { + profile, override, url, wantHost, wantEnv string + wantError bool + }{ + {"", "", "", "https://api.bandwidth.com", "prod", false}, + {"test", "", "", "https://test.api.bandwidth.com", "test", false}, + {"prod", "test", "", "https://test.api.bandwidth.com", "test", false}, + {"test", "prod", "", "https://api.bandwidth.com", "prod", false}, + {"stage", "", "https://custom.example/", "https://custom.example", "stage", false}, + {"typo", "", "", "", "", true}, + {"prod", "", "missing-scheme", "", "", true}, + } { + t.Run(tc.profile+tc.override+tc.url, func(t *testing.T) { + t.Setenv("BW_API_URL", tc.url) + old := EnvironmentOverride + EnvironmentOverride = tc.override + t.Cleanup(func() { EnvironmentOverride = old }) + tm, env, err := AuthTokenManager(&config.Profile{ClientID: "id", Environment: tc.profile}, "secret", "admin") + if (err != nil) != tc.wantError { + t.Fatalf("error = %v", err) + } + if err != nil { + return + } + if env != tc.wantEnv || tm.TokenURL != tc.wantHost || tm.ProfileName != "admin" { + t.Fatalf("wrong environment or profile: %s %s %s", env, tm.TokenURL, tm.ProfileName) + } + }) + } +} + +func TestMissingCredentialsExitAuth(t *testing.T) { + err := &auth.CredentialError{Reason: "not_logged_in", Profile: "admin"} + if got := ExitCodeForError(err); got != ExitAuth { + t.Fatalf("exit = %d", got) + } +} diff --git a/internal/cmdutil/exitcodes.go b/internal/cmdutil/exitcodes.go index d4baf7a..c6d8b10 100644 --- a/internal/cmdutil/exitcodes.go +++ b/internal/cmdutil/exitcodes.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/Bandwidth/cli/internal/api" + "github.com/Bandwidth/cli/internal/auth" ) // Exit code constants for the bw CLI. @@ -113,6 +114,20 @@ func ExitCodeForError(err error) int { return ExitConflict } var apiErr *api.APIError + var credentialErr *auth.CredentialError + if errors.As(err, &credentialErr) { + return ExitAuth + } + var tokenErr *auth.TokenError + if errors.As(err, &tokenErr) { + if tokenErr.Rejected() { + return ExitAuth + } + if tokenErr.StatusCode == 429 { + return ExitRateLimit + } + return ExitGeneral + } if errors.As(err, &apiErr) { switch apiErr.StatusCode { case 401, 403: diff --git a/internal/cmdutil/helpers.go b/internal/cmdutil/helpers.go index 5af54ea..548b303 100644 --- a/internal/cmdutil/helpers.go +++ b/internal/cmdutil/helpers.go @@ -129,12 +129,12 @@ func loadConfigAndAuth() (*config.Config, *config.Profile, string, error) { p := cfg.ActiveProfileConfig() if p.ClientID == "" { - return nil, nil, "", fmt.Errorf("not logged in — run `band auth login` first") + return nil, nil, "", &auth.CredentialError{Reason: "not_logged_in", Profile: ProfileName(cfg)} } clientSecret, err := auth.GetPassword(p.ClientID) - if err != nil { - return nil, nil, "", fmt.Errorf("credentials not found in keychain for %s — run `band auth login`", p.ClientID) + if err != nil || clientSecret == "" { + return nil, nil, "", &auth.CredentialError{Reason: "credentials_unavailable", Profile: ProfileName(cfg)} } return cfg, p, clientSecret, nil @@ -196,9 +196,36 @@ func authenticate(accountIDOverride string) (*auth.TokenManager, string, string, } apiHost := apiHostForEnvironment(env) tm := auth.NewTokenManager(p.ClientID, clientSecret, apiHost) + tm.ProfileName = ProfileName(cfg) return tm, acctID, env, nil } +// ProfileName returns the active profile label used in remediation commands. +func ProfileName(cfg *config.Config) string { + if cfg.ActiveProfile == "" { + return "default" + } + return cfg.ActiveProfile +} + +// AuthTokenManager resolves the same environment as API commands, without +// requiring an account ID: system-wide credentials can still be verified. +func AuthTokenManager(p *config.Profile, secret, profile string) (*auth.TokenManager, string, error) { + if err := ValidateAPIOverride(); err != nil { + return nil, "", err + } + env, err := resolveEnvironment(p.Environment) + if err != nil { + return nil, "", err + } + if env == "" { + env = "prod" + } + tm := auth.NewTokenManager(p.ClientID, secret, apiHostForEnvironment(env)) + tm.ProfileName = profile + return tm, env, nil +} + // BuildClient returns an authenticated JSON API client. func BuildClient(apiBaseURL, accountIDOverride string) (*api.Client, string, error) { tm, acctID, _, err := authenticate(accountIDOverride) @@ -293,6 +320,7 @@ func InsightsClient(accountIDOverride string) (*api.Client, string, error) { ui.Warnf("Bandwidth Insights has no test environment — this request hits PRODUCTION data regardless of --environment.") } tm := auth.NewTokenManager(p.ClientID, clientSecret, apiHostForEnvironment("prod")) + tm.ProfileName = ProfileName(cfg) return api.NewClient(insightsHost()+"/api", tm), acctID, nil } @@ -334,5 +362,6 @@ func MessagingClient(accountIDOverride string) (*api.Client, string, error) { } // Always mint the token against prod (apiHostForEnvironment("prod")). tm := auth.NewTokenManager(p.ClientID, clientSecret, apiHostForEnvironment("prod")) + tm.ProfileName = ProfileName(cfg) return api.NewClient(messagingHost()+"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/api/v2", tm), acctID, nil }