diff --git a/internal/api/auth_resolve.go b/internal/api/auth_resolve.go new file mode 100644 index 0000000..6acbffe --- /dev/null +++ b/internal/api/auth_resolve.go @@ -0,0 +1,267 @@ +package api + +import ( + "fmt" + "os" + "strings" + + "github.com/spf13/viper" + + "github.com/klaassen-consulting/jc/internal/config" + "github.com/klaassen-consulting/jc/internal/keychain" +) + +// ResolvedAuth is the single source of truth for "what auth will jc +// actually use, and where did it come from?" — used by both NewClient +// (which builds the HTTP client from the resolved values) and by +// diagnostic surfaces like `jc doctor` (which render the attribution +// for the operator). +// +// Background: PR #42 (KLA-439, jc doctor) duplicated NewClient's +// precedence logic in cmd/doctor.go's collectAuth. Across the next +// 12 commits Bugbot found 13 distinct edges where the two +// implementations drifted (auth method labels, OAuth fallback to +// api_key, keychain-ref resolution failures, --api-key vs JC_API_KEY +// precedence, ...). Pulling resolution into the api package +// eliminates the entire class by construction: one decision tree, two +// consumers, structurally impossible to disagree. +type ResolvedAuth struct { + // Method is the auth scheme the resolved client will actually use. + // "api_key" | "service_account" | "api_key (service_account fallback)" + // | "api_key (service_account fallback, client_secret keychain unavailable)" + // — when service_account is configured but creds fall back to + // api_key, the method label surfaces both facts so the operator + // sees what happened without having to triage. + Method string + + // Source is the operator-facing description of *where* the active + // credential came from. Examples: + // "--api-key flag" + // "JC_API_KEY env" + // "keychain (jc/default)" + // "profile config (plaintext)" + // "service_account (OAuth)" + // "service_account (client_secret keychain unavailable: jc/default)" + // "service_account (no client credentials)" + // "(unset)" + Source string + + // Fingerprint is "****abcd" — last 4 chars of the active credential + // (api_key for api_key auth, client_id for OAuth). Empty when no + // credential resolved. + Fingerprint string + + // OrgID is the JumpCloud organization ID, resolved via the same + // precedence as config.OrgID(). Empty when not configured. + OrgID string + + // OrgIDSource describes where OrgID came from: + // "JC_ORG_ID env" | "top-level config" | "profile config" + OrgIDSource string + + // Internal fields used by NewClient to build the live transport. + // Unexported so external callers (doctor, audit verifiers) can't + // accidentally leak the raw key. + + // apiKey is the plaintext credential when Method resolves to an + // api_key path. Empty for OAuth and unresolved cases. + apiKey string + + // tokenCache is non-nil when Method is "service_account" (OAuth + // happy path). The cache encapsulates client_id + client_secret + // for lazy token fetching. + tokenCache *TokenCache +} + +// Hint carries disambiguating signals only a caller with cobra/pflag +// state can supply. Fields are all optional — the zero value works +// for callers (like NewClient) that don't have the information. +// +// Why this lives in the api package: ResolveActiveAuth is the single +// source of truth, and the api_key flag-vs-env disambiguation is one +// of the precedence questions it must answer. But pflag itself is a +// cmd-package concern, so the caller plumbs the .Changed bit through +// a typed hint rather than the api package importing pflag. +type Hint struct { + // APIKeyFlagChanged is true when the operator passed --api-key on + // the command line (cobra pflag .Changed bit). When set AND the + // resolved api_key value is non-empty, ResolveActiveAuth attributes + // the source to "--api-key flag" rather than "JC_API_KEY env" + // — matching cobra/viper precedence (flag overrides env, even when + // values happen to match). + APIKeyFlagChanged bool +} + +// ResolveActiveAuth returns the active auth resolution with provenance. +// Decision tree mirrors api.NewClient() exactly (because NewClient +// now calls it). The cmd/doctor.go collectAuth function consumes this +// and copies fields into its display struct — no precedence walking +// outside the api package. +// +// Decision order: +// 1. AuthMethod == "service_account" with valid client_id + client_secret +// → OAuth Bearer. +// 2. AuthMethod == "service_account" without an api_key fallback AND +// client_secret is a failed keychain reference → report the keychain +// failure (operator's actionable cause). +// 3. AuthMethod == "service_account" without an api_key fallback AND +// no special cause → report "no client credentials" (misconfig). +// 4. AuthMethod == "service_account" with creds missing AND an +// api_key resolves → silently fall back to api_key, re-label method +// so the report doesn't claim OAuth. +// 5. AuthMethod == "api_key" (or fallback from 4) → resolve key from +// flag / env / keychain / profile config. +// 6. Nothing configured → Source = "(unset)". +func ResolveActiveAuth(hint Hint) ResolvedAuth { + r := ResolvedAuth{Method: config.AuthMethod()} + profile := config.ActiveProfile() + + r.OrgID, r.OrgIDSource = resolveOrgID(profile) + + // Determine whether an api_key WOULD resolve to a real value. + // This is the structurally-correct check (the one cmd/doctor's + // hasAPIKey got wrong in finding #13): a keychain:// ref that + // fails to resolve is NOT a fallback — it produces no usable key. + envKey := os.Getenv("JC_API_KEY") + flagOrEnv := viper.GetString("api_key") + profileRaw := viper.GetString("profiles." + profile + ".api_key") + resolvedAPIKey, resolvedAPIKeySource := resolveAPIKeyChain(profile, hint, flagOrEnv, envKey, profileRaw) + hasAPIKey := resolvedAPIKey != "" + + // --- Service-account branch --------------------------------------------- + if r.Method == "service_account" { + clientID := config.ClientID() + clientSecretRaw := viper.GetString("profiles." + profile + ".client_secret") + clientSecret := config.ClientSecret() + + oauthAvailable := clientID != "" && clientSecret != "" + keychainFailed := clientID != "" && + strings.HasPrefix(clientSecretRaw, "keychain://") && + clientSecret == "" + + switch { + case oauthAvailable: + r.Source = "service_account (OAuth)" + r.Fingerprint = Fingerprint(clientID) + r.tokenCache = NewTokenCache(clientID, clientSecret) + return r + case !hasAPIKey && keychainFailed: + ref := strings.TrimPrefix(clientSecretRaw, "keychain://") + r.Source = fmt.Sprintf("service_account (client_secret keychain unavailable: %s)", ref) + r.Fingerprint = Fingerprint(clientID) + return r + case !hasAPIKey: + r.Source = "service_account (no client credentials)" + return r + default: + // Silent fallback to api_key. Re-label method so the report + // surfaces both that service_account is the configured intent + // AND that jc is actually using x-api-key. + if keychainFailed { + r.Method = "api_key (service_account fallback, client_secret keychain unavailable)" + } else { + r.Method = "api_key (service_account fallback)" + } + // Fall through to api_key attribution below. + } + } + + // --- API-key branch ----------------------------------------------------- + if resolvedAPIKey != "" { + r.apiKey = resolvedAPIKey + r.Source = resolvedAPIKeySource + r.Fingerprint = Fingerprint(resolvedAPIKey) + return r + } + + // resolvedAPIKey is empty but resolveAPIKeyChain may have populated + // a source label anyway (the keychain-miss case: the operator + // configured a keychain:// ref that couldn't be resolved). Surface + // that as the source + "(keychain unavailable)" fingerprint so the + // report tells the operator their intent AND its failure — distinct + // from "(unset)" which would suggest nothing was configured. + if resolvedAPIKeySource != "" { + r.Source = resolvedAPIKeySource + r.Fingerprint = "(keychain unavailable)" + return r + } + + // Nothing resolved. + r.Source = "(unset)" + return r +} + +// resolveAPIKeyChain returns the resolved api_key (post-keychain) and +// a source label. Returns ("", "") when no key resolves. +// +// Precedence (mirrors config.APIKey() + the flag-vs-env disambiguation +// doctor needs): +// 1. --api-key flag (when hint.APIKeyFlagChanged AND viper value non-empty) +// 2. JC_API_KEY env (when raw env matches the resolved viper value) +// 3. profile config: keychain:// ref → resolve via keychain package +// 4. profile config: plaintext +func resolveAPIKeyChain(profile string, hint Hint, flagOrEnv, envKey, profileRaw string) (string, string) { + switch { + case hint.APIKeyFlagChanged && flagOrEnv != "": + return flagOrEnv, "--api-key flag" + case flagOrEnv != "" && envKey != "" && flagOrEnv == envKey: + return envKey, "JC_API_KEY env" + case flagOrEnv != "": + // Resolved via viper but neither flag nor a matching env — the + // value came from a non-flag, non-direct-env source (e.g. a + // config-level binding). Attribute conservatively. + return flagOrEnv, "viper-resolved (config or env)" + case strings.HasPrefix(profileRaw, "keychain://"): + resolved, err := keychain.Resolve(profileRaw) + ref := strings.TrimPrefix(profileRaw, "keychain://") + if err != nil || resolved == "" { + // Keychain miss for api_key — distinct from "never + // configured." Returning empty resolved value AND a source + // label so the doctor can report the failure cause; the + // caller (ResolveActiveAuth) treats this as no-api-key. + return "", fmt.Sprintf("keychain (%s) — unavailable", ref) + } + return resolved, fmt.Sprintf("keychain (%s)", ref) + case profileRaw != "": + return profileRaw, "profile config (plaintext)" + default: + return "", "" + } +} + +// resolveOrgID mirrors config.OrgID()'s precedence and adds source +// attribution. +func resolveOrgID(profile string) (string, string) { + if topLevel := viper.GetString("org_id"); topLevel != "" { + if os.Getenv("JC_ORG_ID") == topLevel { + return topLevel, "JC_ORG_ID env" + } + return topLevel, "top-level config" + } + if profileOrg := viper.GetString("profiles." + profile + ".org_id"); profileOrg != "" { + return profileOrg, "profile config" + } + return "", "" +} + +// Fingerprint returns "****" + the last 4 chars of s. Used everywhere +// jc displays a credential identifier (audit log signing pubkey, TTY +// step-up prompts, doctor report). Returns "(unset)" for empty input +// so consumers don't have to special-case absent values. +func Fingerprint(s string) string { + if s == "" { + return "(unset)" + } + if len(s) <= 4 { + return "****" + } + return "****" + s[len(s)-4:] +} + +// APIKey returns the resolved api_key for use by NewClient. Empty +// when the active auth method is OAuth or unresolved. +func (r ResolvedAuth) APIKey() string { return r.apiKey } + +// TokenCache returns the resolved OAuth token cache for use by +// NewClient. Nil when the active auth method is api_key. +func (r ResolvedAuth) TokenCache() *TokenCache { return r.tokenCache } diff --git a/internal/api/auth_resolve_test.go b/internal/api/auth_resolve_test.go new file mode 100644 index 0000000..290c49b --- /dev/null +++ b/internal/api/auth_resolve_test.go @@ -0,0 +1,310 @@ +package api + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/viper" + + "github.com/klaassen-consulting/jc/internal/config" +) + +// resolveAuthFixture writes a config file and re-initializes config so +// ResolveActiveAuth sees the expected state. Returns the path written. +// Mirrors the existing client_test.go setup pattern. +func resolveAuthFixture(t *testing.T, body string) string { + t.Helper() + resetViper() + t.Cleanup(resetViper) + + tmp := t.TempDir() + dir := filepath.Join(tmp, "jc") + cfgPath := filepath.Join(dir, "config.yaml") + t.Setenv("JC_CONFIG", cfgPath) + _ = os.MkdirAll(dir, 0o700) + _ = os.WriteFile(cfgPath, []byte(body), 0o600) + if err := config.Init(); err != nil { + t.Fatalf("config.Init: %v", err) + } + return cfgPath +} + +// --- Fingerprint -------------------------------------------------------- + +func TestFingerprint(t *testing.T) { + cases := []struct{ in, want string }{ + {"", "(unset)"}, + {"abcd", "****"}, + {"abcdefgh", "****efgh"}, + {"sk-ant-1234567890abcd", "****abcd"}, + } + for _, tc := range cases { + if got := Fingerprint(tc.in); got != tc.want { + t.Errorf("Fingerprint(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// --- Service-account branches ------------------------------------------- + +func TestResolveActiveAuth_ServiceAccountOAuth(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + auth_method: service_account + client_id: "test-client-id-abcd" + client_secret: "test-secret" +`) + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if r.Method != "service_account" { + t.Errorf("Method = %q, want service_account", r.Method) + } + if r.Source != "service_account (OAuth)" { + t.Errorf("Source = %q, want 'service_account (OAuth)'", r.Source) + } + if r.Fingerprint != "****abcd" { + t.Errorf("Fingerprint = %q, want '****abcd'", r.Fingerprint) + } + if r.TokenCache() == nil { + t.Error("TokenCache should be populated for OAuth path") + } +} + +// Pre-fix (PR #42 Bugbot #3): a stray JC_API_KEY would mislabel an +// otherwise-OAuth profile. The architectural fix prevents that. +func TestResolveActiveAuth_ServiceAccountStrayEnvKeyDoesntOverride(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + auth_method: service_account + client_id: "id-aaaa" + client_secret: "sec" +`) + t.Setenv("JC_API_KEY", "stray-env-key") + + r := ResolveActiveAuth(Hint{}) + if r.Source != "service_account (OAuth)" { + t.Errorf("Source = %q, want OAuth (stray env must not override)", r.Source) + } +} + +func TestResolveActiveAuth_ServiceAccountFallsBackToAPIKey(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + auth_method: service_account + api_key: "fallback-key-1234" +`) + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if !strings.Contains(r.Method, "api_key") { + t.Errorf("Method = %q, want fallback label", r.Method) + } + if !strings.Contains(r.Method, "fallback") { + t.Errorf("Method = %q, want explicit fallback label", r.Method) + } + if r.Fingerprint != "****1234" { + t.Errorf("Fingerprint = %q, want '****1234'", r.Fingerprint) + } + if r.APIKey() == "" { + t.Error("APIKey should be populated on fallback path") + } +} + +func TestResolveActiveAuth_ServiceAccountNoCredsAtAll(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + auth_method: service_account +`) + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if r.Source != "service_account (no client credentials)" { + t.Errorf("Source = %q, want 'service_account (no client credentials)'", r.Source) + } + if r.APIKey() != "" || r.TokenCache() != nil { + t.Error("no-creds case should produce neither APIKey nor TokenCache") + } +} + +// --- API-key branches --------------------------------------------------- + +func TestResolveActiveAuth_FlagBeatsEnvWhenEqual(t *testing.T) { + resolveAuthFixture(t, "active_profile: default\n") + t.Setenv("JC_API_KEY", "same-value-1234") + + r := ResolveActiveAuth(Hint{APIKeyFlagChanged: true}) + if r.Source != "--api-key flag" { + t.Errorf("Source = %q, want '--api-key flag' when flag is set", r.Source) + } +} + +func TestResolveActiveAuth_JCAPIKeyEnv(t *testing.T) { + resolveAuthFixture(t, "active_profile: default\n") + t.Setenv("JC_API_KEY", "env-key-9876") + + r := ResolveActiveAuth(Hint{}) + if r.Source != "JC_API_KEY env" { + t.Errorf("Source = %q, want 'JC_API_KEY env'", r.Source) + } + if r.Fingerprint != "****9876" { + t.Errorf("Fingerprint = %q, want '****9876'", r.Fingerprint) + } +} + +func TestResolveActiveAuth_PlaintextProfile(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + api_key: "plain-abcd" +`) + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if r.Source != "profile config (plaintext)" { + t.Errorf("Source = %q, want 'profile config (plaintext)'", r.Source) + } + if r.Fingerprint != "****abcd" { + t.Errorf("Fingerprint = %q, want '****abcd'", r.Fingerprint) + } +} + +func TestResolveActiveAuth_Unset(t *testing.T) { + resolveAuthFixture(t, "active_profile: default\n") + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if r.Source != "(unset)" { + t.Errorf("Source = %q, want '(unset)'", r.Source) + } +} + +// --- Bugbot #13 fixture (the architectural-fix proof point) ------------- + +// When a service_account profile has a `client_secret` keychain ref +// that won't resolve AND `api_key` is also a keychain ref that won't +// resolve, the doctor's old hasAPIKey peek (raw non-empty) would have +// said "fallback to api_key" while NewClient would return +// ErrNoAPIKey. With the shared resolver, both layers see the same +// resolution: no auth available, ErrNoAPIKey, and Source surfaces the +// actual cause. +// +// Reference: PR #42 Bugbot finding #13 (BUGBOT_BUG_ID fef17827). +func TestResolveActiveAuth_KeychainRefAPIKeyDoesntCountAsFallback(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + auth_method: service_account + api_key: "keychain://jc/nonexistent-key" +`) + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + // Must NOT claim api_key fallback — the keychain ref doesn't + // resolve, so there's no real api_key available. + if strings.Contains(r.Method, "fallback") { + t.Errorf("Method = %q, want NO fallback (keychain doesn't resolve to a real key)", r.Method) + } + if r.APIKey() != "" { + t.Errorf("APIKey = %q, want empty (keychain miss)", r.APIKey()) + } + if r.TokenCache() != nil { + t.Error("TokenCache should be nil (no client_secret)") + } +} + +// And the matching NewClient assertion — proves the two layers agree. +// This is the cross-package parity test that wouldn't have existed in +// the duplicated-precedence world. +func TestNewClient_KeychainRefAPIKeyReturnsErrNoAPIKey(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + auth_method: service_account + api_key: "keychain://jc/nonexistent-key" +`) + t.Setenv("JC_API_KEY", "") + + _, err := NewClient() + if err == nil { + t.Fatal("expected ErrNoAPIKey (keychain doesn't resolve to a real key), got nil") + } + if err != ErrNoAPIKey { + t.Errorf("got %v, want ErrNoAPIKey", err) + } +} + +// --- Org ID precedence -------------------------------------------------- + +func TestResolveActiveAuth_OrgIDTopLevelBeatsProfile(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +org_id: top-level-7777 +profiles: + default: + api_key: "test-1234" + org_id: profile-9999 +`) + t.Setenv("JC_ORG_ID", "") + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if r.OrgID != "top-level-7777" { + t.Errorf("OrgID = %q, want 'top-level-7777' (top-level beats profile)", r.OrgID) + } + if r.OrgIDSource != "top-level config" { + t.Errorf("OrgIDSource = %q, want 'top-level config'", r.OrgIDSource) + } +} + +func TestResolveActiveAuth_OrgIDEnvWins(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +org_id: top-level-7777 +profiles: + default: + api_key: "test-1234" +`) + t.Setenv("JC_ORG_ID", "env-1111") + t.Setenv("JC_API_KEY", "") + // Re-init viper to pick up the env-binding. + _ = viper.BindEnv("org_id", "JC_ORG_ID") + + r := ResolveActiveAuth(Hint{}) + if r.OrgIDSource != "JC_ORG_ID env" { + t.Errorf("OrgIDSource = %q, want 'JC_ORG_ID env'", r.OrgIDSource) + } +} + +func TestResolveActiveAuth_OrgIDProfileFallback(t *testing.T) { + resolveAuthFixture(t, ` +active_profile: default +profiles: + default: + api_key: "test-1234" + org_id: only-profile-2222 +`) + t.Setenv("JC_ORG_ID", "") + t.Setenv("JC_API_KEY", "") + + r := ResolveActiveAuth(Hint{}) + if r.OrgID != "only-profile-2222" { + t.Errorf("OrgID = %q, want 'only-profile-2222'", r.OrgID) + } + if r.OrgIDSource != "profile config" { + t.Errorf("OrgIDSource = %q, want 'profile config'", r.OrgIDSource) + } +} diff --git a/internal/api/client.go b/internal/api/client.go index deba325..2a05034 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -12,7 +12,6 @@ import ( "github.com/spf13/viper" - "github.com/klaassen-consulting/jc/internal/config" "github.com/klaassen-consulting/jc/internal/version" ) @@ -43,25 +42,22 @@ type Client struct { TokenCache *TokenCache } -// NewClient creates a new API client using the currently configured credentials. -// For service accounts, it creates an OAuth bearer-token client. -// For API key auth, it creates an x-api-key client. -// Returns ErrNoAPIKey if no credentials are available. +// NewClient creates a new API client using the currently configured +// credentials. Decision tree shared with ResolveActiveAuth (which +// also drives the operator-facing reporting in `jc doctor`) — the +// single source of truth lives in auth_resolve.go. +// +// Returns ErrNoAPIKey when no usable credential resolves (no OAuth +// client creds, no api_key, or only an unresolvable keychain reference). func NewClient() (*Client, error) { - if config.AuthMethod() == "service_account" { - clientID := config.ClientID() - clientSecret := config.ClientSecret() - if clientID != "" && clientSecret != "" { - tc := NewTokenCache(clientID, clientSecret) - return NewClientWithToken(tc), nil - } + r := ResolveActiveAuth(Hint{}) + if tc := r.TokenCache(); tc != nil { + return NewClientWithToken(tc), nil } - - key := config.APIKey() - if key == "" { - return nil, ErrNoAPIKey + if key := r.APIKey(); key != "" { + return NewClientWithKey(key), nil } - return NewClientWithKey(key), nil + return nil, ErrNoAPIKey } // NewClientWithKey creates a new API client with the given API key. diff --git a/internal/cmd/doctor.go b/internal/cmd/doctor.go index 8c58ab6..99320da 100644 --- a/internal/cmd/doctor.go +++ b/internal/cmd/doctor.go @@ -19,7 +19,6 @@ import ( "github.com/klaassen-consulting/jc/internal/api" "github.com/klaassen-consulting/jc/internal/config" - "github.com/klaassen-consulting/jc/internal/keychain" "github.com/klaassen-consulting/jc/internal/version" ) @@ -258,164 +257,23 @@ func collectConfig() configSection { return cs } -// collectAuth honestly reports where the API key came from, BEFORE -// resolving keychain references. Order matches config.APIKey(): -// 1. --api-key flag (viper's "api_key" key, set via PersistentFlag) -// 2. JC_API_KEY env var (also bound to viper's "api_key" key) -// 3. profile config (plaintext or keychain:// reference) +// collectAuth is a thin presentation adapter over api.ResolveActiveAuth. // -// (1) and (2) are indistinguishable at the viper level since they share -// the binding. We disambiguate by checking whether the binding was set -// via flag — viper's IsSet on a binding name reports whether ANY non- -// default source provided the value, so we additionally compare against -// the raw env value: when the resolved value matches the env value -// (and the env IS set), it's safe to call it env; otherwise it's the -// flag, which has higher precedence in cobra/viper. -// -// Bugbot caught the original ordering, which misattributed a flag -// override to env when the operator happened to set both to the same -// string. The fix checks the flag-set bit first via cobra-provided -// state plumbed in by the caller (we read the persistent flag -// definition's Changed bit). +// Pre-KLA-447 this function held a hand-rolled mirror of NewClient()'s +// precedence — Bugbot found 13 distinct edges across PR #42 where the +// mirror drifted. The fix is structural: api.ResolveActiveAuth is now +// the single source of truth; collectAuth copies its output into the +// display struct. Future maintainers cannot reintroduce a drift here +// because the precedence walking lives in one place. func collectAuth(flagAPIKeySet bool) authSection { - as := authSection{Method: config.AuthMethod()} - profile := config.ActiveProfile() - - envKey := os.Getenv("JC_API_KEY") - flagOrEnv := viper.GetString("api_key") - profileRaw := viper.GetString("profiles." + profile + ".api_key") - hasAPIKey := flagOrEnv != "" || profileRaw != "" - - // api.NewClient() resolution precedence (mirror it exactly so the - // doctor report matches what jc actually does): - // 1. AuthMethod == "service_account" AND client_id + client_secret - // → OAuth Bearer. - // 2. AuthMethod == "service_account" AND creds missing AND an - // api_key resolves → SILENT FALLBACK to api_key (Bugbot caught - // this third-order case where reporting `method: service_account` - // with an x-api-key source was internally inconsistent). - // 3. AuthMethod == "api_key" → resolve key from flag/env/keychain/ - // profile config. - // 4. Nothing configured → no auth. - if as.Method == "service_account" { - // Peek at the raw client_secret to distinguish "not configured" - // from "keychain reference exists but resolution failed" — - // config.ClientSecret() resolves keychain refs transparently - // and returns "" on failure, losing that distinction. - clientID := config.ClientID() - clientSecretRaw := viper.GetString("profiles." + profile + ".client_secret") - clientSecret := config.ClientSecret() - - oauthAvailable := clientID != "" && clientSecret != "" - keychainFailed := clientID != "" && - strings.HasPrefix(clientSecretRaw, "keychain://") && - clientSecret == "" - - switch { - case oauthAvailable: - // (1) Happy OAuth path. - as.Source = "service_account (OAuth)" - as.Fingerprint = fingerprint(clientID) - as.OrgID, as.OrgIDSource = collectOrgID(profile) - return as - case !hasAPIKey && keychainFailed: - // (4a) OAuth would be configured but the client_secret - // keychain is unreadable AND there's no api_key fallback. - // Report the actual cause so the operator fixes the - // keychain rather than re-entering their secret. - ref := strings.TrimPrefix(clientSecretRaw, "keychain://") - as.Source = fmt.Sprintf("service_account (client_secret keychain unavailable: %s)", ref) - as.Fingerprint = fingerprint(clientID) - as.OrgID, as.OrgIDSource = collectOrgID(profile) - return as - case !hasAPIKey: - // (4b) Neither auth path will work and no special cause to - // surface. - as.Source = "service_account (no client credentials)" - as.OrgID, as.OrgIDSource = collectOrgID(profile) - return as - default: - // (2) Silent fallback to api_key. The earlier Bugbot - // finding flagged that reporting `method: service_account` - // with an x-api-key source was inconsistent; this branch - // re-labels method honestly. When the keychain miss caused - // the fallback, surface that in the method label too so - // the operator sees both that their service_account is - // broken AND why. - if keychainFailed { - as.Method = "api_key (service_account fallback, client_secret keychain unavailable)" - } else { - as.Method = "api_key (service_account fallback)" - } - } - } - - switch { - case flagAPIKeySet && flagOrEnv != "": - // Flag overrides env in cobra precedence. Reported as flag - // even when env happens to have the same value. - as.Source = "--api-key flag" - as.Fingerprint = fingerprint(flagOrEnv) - case flagOrEnv != "" && envKey != "" && flagOrEnv == envKey: - as.Source = "JC_API_KEY env" - as.Fingerprint = fingerprint(envKey) - case flagOrEnv != "": - // Active value differs from env (or env is unset) and the flag - // wasn't set — only path that fits is the profile config that - // viper auto-merged into the key. Fall through to profile branch - // below; we'd never get here in practice because the env path - // above would have matched. Belt-and-suspenders. - as.Source = "viper-resolved (config or env)" - as.Fingerprint = fingerprint(flagOrEnv) - case strings.HasPrefix(profileRaw, "keychain://"): - as.Source = fmt.Sprintf("keychain (%s)", strings.TrimPrefix(profileRaw, "keychain://")) - if resolved, err := keychain.Resolve(profileRaw); err == nil { - as.Fingerprint = fingerprint(resolved) - } else { - as.Fingerprint = "(keychain unavailable)" - } - case profileRaw != "": - as.Source = "profile config (plaintext)" - as.Fingerprint = fingerprint(profileRaw) - case as.Method == "service_account": - // service_account profile without valid client creds — neither - // auth path will work. Surface it honestly so the operator - // knows to fix the client_id/client_secret. - as.Source = "service_account (no client credentials)" - default: - as.Source = "(unset)" - } - - as.OrgID, as.OrgIDSource = collectOrgID(profile) - return as -} - -// collectOrgID returns the resolved org ID + source. Mirrors -// config.OrgID()'s precedence exactly: -// -// 1. viper "org_id" — picks up JC_ORG_ID env (via BindEnv), a -// top-level `org_id:` in config.yaml, or a flag bound to "org_id". -// 2. profile config (profiles..org_id). -// -// Bugbot caught the original implementation reading os.Getenv("JC_ORG_ID") -// directly, which missed the top-level config-file case — the doctor -// would have reported no org ID while config.OrgID() (and every other -// jc command) still resolved one. -func collectOrgID(profile string) (string, string) { - if topLevel := viper.GetString("org_id"); topLevel != "" { - // Attribute to env when the env var matches the resolved value - // (most common case); fall back to a top-level-config attribution - // otherwise. Cannot distinguish flag vs config without a - // pflag.Changed peek, and there's no --org-id flag today. - if os.Getenv("JC_ORG_ID") == topLevel { - return topLevel, "JC_ORG_ID env" - } - return topLevel, "top-level config" - } - if profileOrg := viper.GetString("profiles." + profile + ".org_id"); profileOrg != "" { - return profileOrg, "profile config" + r := api.ResolveActiveAuth(api.Hint{APIKeyFlagChanged: flagAPIKeySet}) + return authSection{ + Method: r.Method, + Source: r.Source, + Fingerprint: r.Fingerprint, + OrgID: r.OrgID, + OrgIDSource: r.OrgIDSource, } - return "", "" } func collectAPI() apiSection { diff --git a/internal/cmd/doctor_test.go b/internal/cmd/doctor_test.go index 9dc9d55..1f3fb4b 100644 --- a/internal/cmd/doctor_test.go +++ b/internal/cmd/doctor_test.go @@ -569,12 +569,12 @@ profiles: t.Setenv("JC_ORG_ID", "") _ = viper.BindEnv("org_id", "JC_ORG_ID") - orgID, source := collectOrgID("default") - if orgID != "top-level-org-7777" { - t.Errorf("org_id = %q, want 'top-level-org-7777' (top-level beats profile, matching config.OrgID() precedence)", orgID) + a := collectAuth(false) + if a.OrgID != "top-level-org-7777" { + t.Errorf("OrgID = %q, want 'top-level-org-7777' (top-level beats profile, matching config.OrgID() precedence)", a.OrgID) } - if source != "top-level config" { - t.Errorf("source = %q, want 'top-level config'", source) + if a.OrgIDSource != "top-level config" { + t.Errorf("OrgIDSource = %q, want 'top-level config'", a.OrgIDSource) } } @@ -588,12 +588,12 @@ org_id: top-level-org-7777 t.Setenv("JC_ORG_ID", "env-org-1111") _ = viper.BindEnv("org_id", "JC_ORG_ID") - orgID, source := collectOrgID("default") - if orgID != "env-org-1111" { - t.Errorf("org_id = %q, want env value", orgID) + a := collectAuth(false) + if a.OrgID != "env-org-1111" { + t.Errorf("OrgID = %q, want env value", a.OrgID) } - if source != "JC_ORG_ID env" { - t.Errorf("source = %q, want 'JC_ORG_ID env'", source) + if a.OrgIDSource != "JC_ORG_ID env" { + t.Errorf("OrgIDSource = %q, want 'JC_ORG_ID env'", a.OrgIDSource) } }