diff --git a/cmd/config/init_probe.go b/cmd/config/init_probe.go index e56aa144..b6873ac2 100644 --- a/cmd/config/init_probe.go +++ b/cmd/config/init_probe.go @@ -33,15 +33,16 @@ const probeTimeout = 3 * time.Second // // 1. A TAT request using the just-saved credentials. credential.FetchTAT // returns a typed errs.* error (via the shared classifyTATResponseCode) -// only when the server deterministically rejected the credentials — a -// non-zero TAT body code, classified as CategoryConfig / SubtypeInvalidClient -// (10003 / 10014) or whatever codemeta maps. That typed error is propagated -// so the root dispatcher renders the canonical envelope and `config init` -// exits non-zero — identical to how every other token-resolving command -// reports the same bad credentials. Ambiguous failures (transport errors, -// HTTP non-200, JSON parse errors, timeouts) come back as raw untyped -// errors and are swallowed (return nil), so valid configurations are never -// disturbed by upstream noise. errs.IsTyped is the discriminator. +// only when the unified Token Endpoint deterministically rejected the +// credentials — an OAuth2 invalid_client / unauthorized_client classified as +// CategoryConfig / SubtypeInvalidClient, or whatever codemeta maps. That +// typed error is propagated so the root dispatcher renders the canonical +// envelope and `config init` exits non-zero — identical to how every other +// token-resolving command reports the same bad credentials. Ambiguous +// failures (transport errors, transient 5xx/server_error, JSON parse errors, +// timeouts) come back as raw untyped errors and are swallowed (return nil), +// so valid configurations are never disturbed by upstream noise. +// errs.IsTyped is the discriminator. // // 2. If TAT succeeded, a POST to the probe endpoint is fired. The outcome of // that call (success, server error, timeout, parse failure) is always diff --git a/cmd/config/init_probe_test.go b/cmd/config/init_probe_test.go index 097817d7..f4156a73 100644 --- a/cmd/config/init_probe_test.go +++ b/cmd/config/init_probe_test.go @@ -31,10 +31,10 @@ type fakeRT struct { func (f *fakeRT) RoundTrip(req *http.Request) (*http.Response, error) { switch { - case strings.HasSuffix(req.URL.Path, "/auth/v3/tenant_access_token/internal"): + case strings.HasSuffix(req.URL.Path, "/oauth/v3/token"): f.tatCalls++ if f.tatHandler == nil { - return jsonResp(200, `{"code":0,"tenant_access_token":"t-ok"}`), nil + return jsonResp(200, `{"code":0,"access_token":"t-ok","token_type":"Bearer"}`), nil } return f.tatHandler(req) case strings.HasSuffix(req.URL.Path, "/application/v6/larksuite_cli_app/probe"): @@ -84,14 +84,15 @@ func fakeFactory(t *testing.T, rt http.RoundTripper) (*cmdutil.Factory, *bytes.B } // assertConfigRejection asserts runProbe propagated a deterministic credential -// rejection: a *errs.ConfigError (CategoryConfig / SubtypeInvalidClient) with -// the expected upstream code. This is the same typed error every other -// token-resolving command returns for the same bad credentials, and nothing is -// written to stderr (the root dispatcher renders the envelope). -func assertConfigRejection(t *testing.T, err error, errBuf *bytes.Buffer, wantCode int) { +// rejection: a *errs.ConfigError (CategoryConfig / SubtypeInvalidClient). This +// is the same typed error every other token-resolving command returns for the +// same bad credentials, and nothing is written to stderr (the root dispatcher +// renders the envelope). The numeric code is not asserted: the unified v3 Token +// Endpoint reports invalid_client via the OAuth2 error string, not a Lark code. +func assertConfigRejection(t *testing.T, err error, errBuf *bytes.Buffer) { t.Helper() if err == nil { - t.Fatalf("expected *errs.ConfigError (code %d), got nil", wantCode) + t.Fatal("expected *errs.ConfigError, got nil") } var cfgErr *errs.ConfigError if !errors.As(err, &cfgErr) { @@ -103,9 +104,6 @@ func assertConfigRejection(t *testing.T, err error, errBuf *bytes.Buffer, wantCo if cfgErr.Subtype != errs.SubtypeInvalidClient { t.Errorf("Subtype = %q, want %q", cfgErr.Subtype, errs.SubtypeInvalidClient) } - if cfgErr.Code != wantCode { - t.Errorf("Code = %d, want %d", cfgErr.Code, wantCode) - } if errBuf.Len() != 0 { t.Errorf("runProbe must not write to stderr, got: %q", errBuf.String()) } @@ -123,11 +121,13 @@ func assertSilent(t *testing.T, err error, errBuf *bytes.Buffer) { } } -// 10003 (bad / non-existent app_id) → ConfigError/InvalidClient, propagated. -func TestRunProbe_TATCode10003_ReturnsConfigError(t *testing.T) { +// invalid_client (bad / non-existent app_id or wrong secret) → the v3 Token +// Endpoint returns HTTP 400 with the OAuth2 error → ConfigError/InvalidClient, +// propagated. The probe endpoint must not be called when TAT fails. +func TestRunProbe_TATInvalidClient_ReturnsConfigError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return jsonResp(200, `{"code":10003,"msg":"invalid param"}`), nil + return jsonResp(400, `{"error":"invalid_client","error_description":"The client secret is invalid.","code":20002}`), nil }, } f, errBuf := fakeFactory(t, rt) @@ -137,28 +137,27 @@ func TestRunProbe_TATCode10003_ReturnsConfigError(t *testing.T) { if rt.probeCalls != 0 { t.Error("probe endpoint must not be called when TAT fails") } - assertConfigRejection(t, err, errBuf, 10003) + assertConfigRejection(t, err, errBuf) } -// 10014 (real app_id + wrong secret) → ConfigError/InvalidClient via codemeta — -// the most common real-world rejection, propagated. -func TestRunProbe_TATCode10014_ReturnsConfigError(t *testing.T) { +// unauthorized_client is treated as the same credential rejection, propagated. +func TestRunProbe_TATUnauthorizedClient_ReturnsConfigError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return jsonResp(200, `{"code":10014,"msg":"app secret invalid"}`), nil + return jsonResp(401, `{"error":"unauthorized_client","error_description":"client not authorized"}`), nil }, } f, errBuf := fakeFactory(t, rt) - assertConfigRejection(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), errBuf, 10014) + assertConfigRejection(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), errBuf) } -// Any non-zero body code is a deterministic rejection and propagates (typed). -// An unrecognized code falls back to *errs.APIError via BuildAPIError — still -// typed, so the probe still surfaces it rather than swallowing. -func TestRunProbe_TATUnknownBodyCode_Propagates(t *testing.T) { +// Any other deterministic client-side OAuth error (e.g. invalid_scope) falls +// back to *errs.APIError via BuildAPIError — still typed, so the probe surfaces +// it rather than swallowing — but is not a credential (ConfigError) rejection. +func TestRunProbe_TATOtherClientError_Propagates(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return jsonResp(200, `{"code":99999,"msg":"future-unknown"}`), nil + return jsonResp(400, `{"code":20068,"error":"invalid_scope","error_description":"unauthorized scope"}`), nil }, } f, errBuf := fakeFactory(t, rt) diff --git a/internal/core/types.go b/internal/core/types.go index cf842e6a..26236df7 100644 --- a/internal/core/types.go +++ b/internal/core/types.go @@ -22,6 +22,12 @@ func ParseBrand(value string) LarkBrand { return BrandFeishu } +// OAuthTokenV3Path is the unified OAuth 2.0 Token Endpoint path on the accounts +// domain. It serves every grant type (client_credentials for TAT, +// authorization_code / device_code / refresh_token for UAT) and replaces the +// legacy per-token endpoints (e.g. /open-apis/auth/v3/tenant_access_token/internal). +const OAuthTokenV3Path = "/oauth/v3/token" + // Endpoints holds resolved endpoint URLs for different Lark services. type Endpoints struct { Open string // e.g. "https://open.feishu.cn" diff --git a/internal/core/types_test.go b/internal/core/types_test.go index 72f33170..55cdd1f1 100644 --- a/internal/core/types_test.go +++ b/internal/core/types_test.go @@ -42,6 +42,11 @@ func TestResolveEndpoints_EmptyDefaultsToFeishu(t *testing.T) { if ep.Open != "https://open.feishu.cn" { t.Errorf("Open = %q, want feishu.cn for empty brand", ep.Open) } + // The unified OAuth v3 Token Endpoint mints TAT on the accounts domain; + // pin the default-brand host so a stray non-production domain revert is caught. + if ep.Accounts != "https://accounts.feishu.cn" { + t.Errorf("Accounts = %q, want accounts.feishu.cn for empty brand", ep.Accounts) + } } func TestResolveOpenBaseURL(t *testing.T) { diff --git a/internal/credential/default_provider.go b/internal/credential/default_provider.go index 9482b284..d7401b62 100644 --- a/internal/credential/default_provider.go +++ b/internal/credential/default_provider.go @@ -19,33 +19,44 @@ import ( extcred "github.com/larksuite/cli/extension/credential" ) -// classifyTATResponseCode wraps a non-zero TAT endpoint response code into the -// canonical typed error. The TAT mint endpoint reports invalid credentials -// with two distinct codes: +// classifyTATResponseCode wraps a deterministic (non-transient) failure from the +// unified Token Endpoint into the canonical typed errs.* error. The v3 endpoint +// reports failures using the OAuth 2.0 model — an `error` string plus an +// optional numeric `code` — instead of the legacy `{code, msg}` shape. // -// - 10003: bad app_id format or non-existent app_id ("invalid param") -// - 10014: invalid app_secret ("app secret invalid") -// -// Both surface as CategoryConfig/InvalidClient from the user's perspective — -// the configured credentials cannot mint a tenant access token. 10014 is -// globally mapped in codemeta (TAT-mint-specific variant of OAuth 99991543). -// 10003 is NOT globally mapped because in other Lark endpoints it carries -// unrelated semantics (e.g. task API uses 10003 for permission denied), so -// the override stays local to this TAT call site instead of leaking into the -// shared codemeta table. -func classifyTATResponseCode(code int, msg, brand, appID string) error { - if code == 10003 { +// invalid_client / unauthorized_client mean the configured app_id/app_secret +// cannot mint a token; from the user's perspective that is the same actionable +// CategoryConfig/InvalidClient failure the legacy 10003/10014 codes produced. +// Every other deterministic error falls through to BuildAPIError, which still +// yields a typed error so probe callers (errs.IsTyped) surface it rather than +// swallowing it. Transient/server-side failures (5xx / server_error) are +// filtered out by FetchTAT before this is called, so they stay untyped. +func classifyTATResponseCode(code int, oauthErr, errDesc, brand, appID string) error { + msg := errDesc + if msg == "" { + msg = oauthErr + } + switch oauthErr { + case "invalid_client", "unauthorized_client": return errs.NewConfigError(errs.SubtypeInvalidClient, "%s", msg). WithCode(code). WithHint("%s", errclass.ConfigHint(errs.SubtypeInvalidClient)) } - return errclass.BuildAPIError(map[string]any{ + if err := errclass.BuildAPIError(map[string]any{ "code": code, "msg": msg, }, errclass.ClassifyContext{ Brand: brand, AppID: appID, - }) + }); err != nil { + return err + } + // BuildAPIError returns nil for code 0 (Feishu's success convention), but this + // function is only reached once FetchTAT has ruled out success — a non-credential + // OAuth error (e.g. invalid_scope) can arrive with code 0 and is still a + // deterministic rejection. Back it with a typed APIError so callers never receive + // the ("", nil) "empty token, no error" pair. + return errs.NewAPIError(errs.SubtypeUnknown, "%s", msg).WithCode(code) } // DefaultAccountProvider resolves account from config.json via keychain. @@ -146,8 +157,8 @@ func (p *DefaultTokenProvider) resolveUAT(ctx context.Context) (*TokenResult, er return &TokenResult{Token: token, Scopes: scopes}, nil } -// resolveTAT resolves a tenant access token. Result is cached after first call. -// NOTE: Uses sync.Once — only the context from the first call is used. +// resolveTAT resolves a tenant access token. The result is cached after the first +// call via sync.Once — only the context from the first call is used. func (p *DefaultTokenProvider) resolveTAT(ctx context.Context) (*TokenResult, error) { p.tatOnce.Do(func() { p.tatResult, p.tatErr = p.doResolveTAT(ctx) diff --git a/internal/credential/default_provider_test.go b/internal/credential/default_provider_test.go index 46e21f06..057f1dba 100644 --- a/internal/credential/default_provider_test.go +++ b/internal/credential/default_provider_test.go @@ -19,18 +19,16 @@ func TestDefaultAccountProvider_Implements(t *testing.T) { var _ DefaultAccountResolver = &DefaultAccountProvider{} } -// TestClassifyTATResponseCode_10003_MapsToInvalidClient pins that the TAT -// endpoint's "invalid param" code surfaces as CategoryConfig/InvalidClient. -// Reason: a bad or non-existent app_id triggers 10003 on the TAT mint endpoint, -// which from the user's perspective is the same actionable failure as 10014 -// ("app secret invalid") — both mean the configured credentials cannot mint a -// tenant access token. The global codemeta intentionally does not map 10003 -// because in other Lark endpoints 10003 carries unrelated semantics (e.g. task -// API uses it for permission denied), so the override is local to this site. -func TestClassifyTATResponseCode_10003_MapsToInvalidClient(t *testing.T) { - err := classifyTATResponseCode(10003, "invalid param", "feishu", "cli_app_x") +// TestClassifyTATResponseCode_InvalidClient_MapsToInvalidClient pins that the +// unified Token Endpoint's OAuth2 invalid_client error surfaces as +// CategoryConfig/InvalidClient — the configured app_id/app_secret cannot mint a +// tenant access token, the same actionable failure the legacy 10003/10014 codes +// produced. The numeric code is intentionally not asserted: the v3 endpoint may +// return invalid_client with no Lark code (code defaults to 0). +func TestClassifyTATResponseCode_InvalidClient_MapsToInvalidClient(t *testing.T) { + err := classifyTATResponseCode(0, "invalid_client", "client authentication failed", "feishu", "cli_app_x") if err == nil { - t.Fatal("expected non-nil error for code=10003") + t.Fatal("expected non-nil error for invalid_client") } var cfgErr *errs.ConfigError if !errors.As(err, &cfgErr) { @@ -42,22 +40,16 @@ func TestClassifyTATResponseCode_10003_MapsToInvalidClient(t *testing.T) { if cfgErr.Subtype != errs.SubtypeInvalidClient { t.Errorf("Subtype = %q, want %q", cfgErr.Subtype, errs.SubtypeInvalidClient) } - if cfgErr.Code != 10003 { - t.Errorf("Code = %d, want 10003", cfgErr.Code) - } if cfgErr.Hint == "" { t.Error("Hint must be non-empty so the user gets a recovery action") } } -// TestClassifyTATResponseCode_10014_RoutesViaCodeMeta pins that 10014 still -// goes through the global BuildAPIError path (codemeta entry) so the override -// for 10003 does not regress the existing mapping. -func TestClassifyTATResponseCode_10014_RoutesViaCodeMeta(t *testing.T) { - err := classifyTATResponseCode(10014, "app secret invalid", "feishu", "cli_app_x") - if err == nil { - t.Fatal("expected non-nil error for code=10014") - } +// TestClassifyTATResponseCode_UnauthorizedClient_MapsToInvalidClient pins that +// unauthorized_client is treated as the same credential failure as +// invalid_client. +func TestClassifyTATResponseCode_UnauthorizedClient_MapsToInvalidClient(t *testing.T) { + err := classifyTATResponseCode(0, "unauthorized_client", "client not authorized", "feishu", "cli_app_x") var cfgErr *errs.ConfigError if !errors.As(err, &cfgErr) { t.Fatalf("expected *errs.ConfigError, got %T: %v", err, err) @@ -65,21 +57,38 @@ func TestClassifyTATResponseCode_10014_RoutesViaCodeMeta(t *testing.T) { if cfgErr.Subtype != errs.SubtypeInvalidClient { t.Errorf("Subtype = %q, want %q", cfgErr.Subtype, errs.SubtypeInvalidClient) } - if cfgErr.Code != 10014 { - t.Errorf("Code = %d, want 10014", cfgErr.Code) - } } -// TestClassifyTATResponseCode_UnknownCodeFallsThrough pins that codes outside -// the credential set fall through to the generic BuildAPIError fallback -// (CategoryAPI/SubtypeUnknown) — the override is narrow and intentional. -func TestClassifyTATResponseCode_UnknownCodeFallsThrough(t *testing.T) { - err := classifyTATResponseCode(99999999, "some unknown failure", "feishu", "cli_app_x") +// TestClassifyTATResponseCode_OtherErrorFallsThrough pins that OAuth errors +// outside the credential set fall through to the generic BuildAPIError fallback +// — still typed, but not a ConfigError. The mapping is narrow and intentional. +func TestClassifyTATResponseCode_OtherErrorFallsThrough(t *testing.T) { + err := classifyTATResponseCode(20068, "invalid_scope", "unauthorized scope", "feishu", "cli_app_x") if err == nil { - t.Fatal("expected non-nil error for unmapped code") + t.Fatal("expected non-nil error for invalid_scope") } var cfgErr *errs.ConfigError if errors.As(err, &cfgErr) { - t.Fatalf("unmapped code must not be classified as ConfigError, got %T", err) + t.Fatalf("invalid_scope must not be classified as ConfigError, got %T", err) + } +} + +// TestClassifyTATResponseCode_CodeZeroOtherError_StillTyped pins the code-0 +// backstop: a non-credential OAuth error (e.g. invalid_scope) that arrives with no +// numeric code (code 0) must still produce a non-nil typed error. BuildAPIError +// returns nil for code 0 (Feishu's success convention); without the backstop, +// FetchTAT would surface this deterministic rejection as ("", nil) — an empty token +// with no error. +func TestClassifyTATResponseCode_CodeZeroOtherError_StillTyped(t *testing.T) { + err := classifyTATResponseCode(0, "invalid_scope", "the requested scope is not granted", "feishu", "cli_app_x") + if err == nil { + t.Fatal("expected non-nil error for code-0 invalid_scope (must not be swallowed as success)") + } + if !errs.IsTyped(err) { + t.Fatalf("expected a typed errs.* error, got %T %v", err, err) + } + var cfgErr *errs.ConfigError + if errors.As(err, &cfgErr) { + t.Fatalf("code-0 invalid_scope must not be a ConfigError, got %T", err) } } diff --git a/internal/credential/tat_fetch.go b/internal/credential/tat_fetch.go index eecaed20..0d916245 100644 --- a/internal/credential/tat_fetch.go +++ b/internal/credential/tat_fetch.go @@ -4,46 +4,47 @@ package credential import ( - "bytes" "context" "encoding/json" "fmt" + "io" "net/http" + "net/url" + "strings" "github.com/larksuite/cli/internal/core" ) -// FetchTAT performs a single HTTP POST to mint a tenant access token with the -// given credentials. It does not read configuration or keychain, so callers -// that already hold plaintext credentials (e.g. the post-`config init` probe) -// can validate them without a second keychain round-trip. +// FetchTAT performs a single HTTP POST to mint a tenant access token via the +// unified OAuth 2.0 Token Endpoint ({accounts}/oauth/v3/token) using the +// client_credentials grant with client_secret_post authentication. It does not +// read configuration or keychain, so callers that already hold plaintext +// credentials (e.g. the post-`config init` probe) can validate them without a +// second keychain round-trip. // -// A non-zero TAT response code means the server inspected the payload and -// rejected the credentials; FetchTAT returns the canonical typed error from -// classifyTATResponseCode — the SAME classification doResolveTAT (and thus -// every token-resolving command) produces, so callers see one consistent -// envelope (CategoryConfig / SubtypeInvalidClient for 10003 / 10014, etc.). -// Transport, HTTP-status and JSON-parse failures are returned raw (untyped), -// leaving them ambiguous; a caller can use errs.IsTyped to tell a deterministic -// credential rejection apart from upstream/transport noise. +// A deterministic client-side rejection (e.g. invalid_client) returns the +// canonical typed error from classifyTATResponseCode — the SAME classification +// doResolveTAT (and thus every token-resolving command) produces, so callers +// see one consistent envelope. Transport failures, unreadable/unparseable +// bodies, and transient server-side failures (5xx / server_error) are returned +// raw (untyped), leaving them ambiguous; a caller can use errs.IsTyped to tell a +// deterministic credential rejection apart from upstream/transport noise. // // The caller owns the context timeout. func FetchTAT(ctx context.Context, httpClient *http.Client, brand core.LarkBrand, appID, appSecret string) (string, error) { ep := core.ResolveEndpoints(brand) - url := ep.Open + "/open-apis/auth/v3/tenant_access_token/internal" + endpoint := ep.Accounts + core.OAuthTokenV3Path - body, err := json.Marshal(map[string]string{ - "app_id": appID, - "app_secret": appSecret, - }) - if err != nil { - return "", fmt.Errorf("failed to marshal TAT request: %w", err) - } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + form := url.Values{} + form.Set("grant_type", "client_credentials") + form.Set("client_id", appID) + form.Set("client_secret", appSecret) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, strings.NewReader(form.Encode())) if err != nil { return "", err } - req.Header.Set("Content-Type", "application/json") + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") resp, err := httpClient.Do(req) if err != nil { @@ -51,20 +52,51 @@ func FetchTAT(ctx context.Context, httpClient *http.Client, brand core.LarkBrand } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("TAT API returned HTTP %d", resp.StatusCode) + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return "", fmt.Errorf("failed to read TAT response: %w", err) } var result struct { - Code int `json:"code"` - Msg string `json:"msg"` - TenantAccessToken string `json:"tenant_access_token"` + Code int `json:"code"` + AccessToken string `json:"access_token"` + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + Msg string `json:"msg"` } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return "", fmt.Errorf("failed to parse TAT response: %w", err) + if err := json.Unmarshal(body, &result); err != nil { + // An unparseable body is ambiguous (covers non-JSON error pages and + // truncated payloads); stay untyped so probe callers treat it as noise. + return "", fmt.Errorf("failed to parse TAT response (HTTP %d): %w", resp.StatusCode, err) } - if result.Code != 0 { - return "", classifyTATResponseCode(result.Code, result.Msg, string(brand), appID) + + if result.Code == 0 && result.AccessToken != "" { + return result.AccessToken, nil } - return result.TenantAccessToken, nil + + // Transient/server-side failures stay untyped so probe callers stay silent and + // retryers can back off; only deterministic client rejections are typed. Covers + // 5xx, HTTP 429 rate-limit, and the OAuth transient error strings (server_error, + // temporarily_unavailable, slow_down) — matching the legacy "non-2xx is noise" + // behavior so a rate-limited probe is not surfaced as a hard credential error. + if resp.StatusCode >= 500 || resp.StatusCode == http.StatusTooManyRequests || + result.Error == "server_error" || result.Error == "temporarily_unavailable" || + result.Error == "slow_down" { + return "", fmt.Errorf("TAT endpoint transient failure (HTTP %d, code=%d, error=%q): %s", + resp.StatusCode, result.Code, result.Error, result.ErrorDescription) + } + + // A 2xx with neither token nor error is a malformed success — ambiguous, untyped. + if result.Code == 0 && result.Error == "" { + return "", fmt.Errorf("TAT response missing access_token (HTTP %d)", resp.StatusCode) + } + + // Prefer the OAuth error_description; fall back to the legacy Lark `msg` so a + // gateway-level {code, msg} response (carrying no OAuth fields) still yields a + // non-empty typed message instead of a bare "API error: [code]". + desc := result.ErrorDescription + if desc == "" { + desc = result.Msg + } + return "", classifyTATResponseCode(result.Code, result.Error, desc, string(brand), appID) } diff --git a/internal/credential/tat_fetch_test.go b/internal/credential/tat_fetch_test.go index 686d98cc..a899d205 100644 --- a/internal/credential/tat_fetch_test.go +++ b/internal/credential/tat_fetch_test.go @@ -44,7 +44,7 @@ func (s *stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) func TestFetchTAT_Success(t *testing.T) { rt := &stubRoundTripper{ respCode: 200, - respBody: `{"code":0,"tenant_access_token":"t-abc","msg":"ok"}`, + respBody: `{"code":0,"access_token":"t-abc","token_type":"Bearer","expires_in":7200}`, } hc := &http.Client{Transport: rt} @@ -55,24 +55,33 @@ func TestFetchTAT_Success(t *testing.T) { if token != "t-abc" { t.Errorf("token = %q, want t-abc", token) } - if rt.gotReq.URL.String() != "https://open.feishu.cn/open-apis/auth/v3/tenant_access_token/internal" { + if rt.gotReq.URL.String() != "https://accounts.feishu.cn/oauth/v3/token" { t.Errorf("url = %s", rt.gotReq.URL.String()) } - if !strings.Contains(rt.gotBody, `"app_id":"cli_app"`) || !strings.Contains(rt.gotBody, `"app_secret":"secret_x"`) { - t.Errorf("request body missing credentials: %s", rt.gotBody) + if ct := rt.gotReq.Header.Get("Content-Type"); ct != "application/x-www-form-urlencoded" { + t.Errorf("Content-Type = %q, want application/x-www-form-urlencoded", ct) + } + // client_secret_post: grant_type + client_id + client_secret in the form body. + for _, want := range []string{"grant_type=client_credentials", "client_id=cli_app", "client_secret=secret_x"} { + if !strings.Contains(rt.gotBody, want) { + t.Errorf("request body missing %q: %s", want, rt.gotBody) + } } } -// 10003 (bad / non-existent app_id, "invalid param") is classified locally by +// invalid_client (wrong app_id/app_secret on the client_credentials grant) is a +// deterministic client-side rejection that FetchTAT routes to // classifyTATResponseCode as CategoryConfig / SubtypeInvalidClient — the same // typed error doResolveTAT (and thus every token-resolving command) returns. -func TestFetchTAT_Code10003_ConfigInvalidClient(t *testing.T) { - rt := &stubRoundTripper{respCode: 200, respBody: `{"code":10003,"msg":"invalid param"}`} +// The v3 endpoint reports it as HTTP 400 with the OAuth2 error body (wrong +// secret → code 20002, unknown app → code 20048). +func TestFetchTAT_InvalidClient_ConfigInvalidClient(t *testing.T) { + rt := &stubRoundTripper{respCode: 400, respBody: `{"error":"invalid_client","error_description":"The client secret is invalid.","code":20002}`} hc := &http.Client{Transport: rt} token, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err == nil { - t.Fatal("expected error for code 10003") + t.Fatal("expected error for invalid_client") } if token != "" { t.Errorf("token = %q, want empty", token) @@ -87,52 +96,115 @@ func TestFetchTAT_Code10003_ConfigInvalidClient(t *testing.T) { if cfgErr.Subtype != errs.SubtypeInvalidClient { t.Errorf("Subtype = %q, want %q", cfgErr.Subtype, errs.SubtypeInvalidClient) } - if cfgErr.Code != 10003 { - t.Errorf("Code = %d, want 10003", cfgErr.Code) - } } -// 10014 ("app secret invalid") — the most common real-world rejection (real -// app_id + wrong secret) — is globally mapped in codemeta to -// CategoryConfig / SubtypeInvalidClient via BuildAPIError. -func TestFetchTAT_Code10014_ConfigInvalidClient(t *testing.T) { - rt := &stubRoundTripper{respCode: 200, respBody: `{"code":10014,"msg":"app secret invalid"}`} - hc := &http.Client{Transport: rt} - - _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") - var cfgErr *errs.ConfigError - if !errors.As(err, &cfgErr) { - t.Fatalf("error not *errs.ConfigError: %T %v", err, err) - } - if cfgErr.Subtype != errs.SubtypeInvalidClient || cfgErr.Code != 10014 { - t.Errorf("got Subtype=%q Code=%d, want invalid_client/10014", cfgErr.Subtype, cfgErr.Code) - } -} - -// Any non-zero body code is a deterministic server-side rejection, so it -// always yields a typed error (errs.IsTyped). An unrecognized code falls back -// to CategoryAPI / SubtypeUnknown via BuildAPIError — still typed, so a probe -// caller still surfaces it rather than silently swallowing. -func TestFetchTAT_UnknownBodyCode_Typed(t *testing.T) { - rt := &stubRoundTripper{respCode: 200, respBody: `{"code":99999,"msg":"future-unknown"}`} +// Any other deterministic client-side OAuth error (e.g. invalid_scope) still +// yields a typed error (errs.IsTyped) via BuildAPIError — so a probe caller +// surfaces it rather than silently swallowing it — but is NOT classified as a +// credential (invalid_client) problem. +func TestFetchTAT_OtherClientError_Typed(t *testing.T) { + rt := &stubRoundTripper{respCode: 400, respBody: `{"code":20068,"error":"invalid_scope","error_description":"unauthorized scope"}`} hc := &http.Client{Transport: rt} _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err == nil { - t.Fatal("expected error for code 99999") + t.Fatal("expected error for invalid_scope") } if !errs.IsTyped(err) { t.Fatalf("expected a typed errs.* error, got %T %v", err, err) } - var apiErr *errs.APIError - if !errors.As(err, &apiErr) { - t.Errorf("unknown code should fall back to *errs.APIError, got %T", err) + var cfgErr *errs.ConfigError + if errors.As(err, &cfgErr) { + t.Errorf("invalid_scope must not be classified as ConfigError/InvalidClient, got %T", err) } } -// Non-2xx HTTP is ambiguous (not a payload-level credential rejection) — it -// must stay UNTYPED so a probe caller treats it as upstream noise and stays -// silent. +// A deterministic OAuth error that arrives WITHOUT a numeric code (code defaults to +// 0) must still surface as a non-nil typed error — never the ("", nil) success pair. +// Guards the code-0 backstop in classifyTATResponseCode: BuildAPIError returns nil +// for code 0, which would otherwise swallow this rejection into an empty-token success. +func TestFetchTAT_OtherClientError_CodeZero_Typed(t *testing.T) { + rt := &stubRoundTripper{respCode: 400, respBody: `{"error":"invalid_scope","error_description":"the requested scope is not granted"}`} + hc := &http.Client{Transport: rt} + + tok, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected non-nil error for code-0 invalid_scope (must not return empty token + nil error)") + } + if tok != "" { + t.Errorf("token = %q, want empty", tok) + } + if !errs.IsTyped(err) { + t.Fatalf("expected a typed errs.* error, got %T %v", err, err) + } +} + +// A gateway-style {code, msg} error (no OAuth error / error_description fields) +// must still surface its msg on the typed error, not degrade to a generic +// "API error: [code]". Guards the legacy-msg fallback in FetchTAT. +func TestFetchTAT_LarkStyleMsg_FallsBackOnTypedError(t *testing.T) { + rt := &stubRoundTripper{respCode: 400, respBody: `{"code":99999,"msg":"app ticket invalid"}`} + hc := &http.Client{Transport: rt} + + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error for {code, msg} response") + } + if !errs.IsTyped(err) { + t.Fatalf("expected a typed errs.* error, got %T %v", err, err) + } + if !strings.Contains(err.Error(), "app ticket invalid") { + t.Errorf("typed error must carry the Lark msg, got: %v", err) + } +} + +// Transient server-side failures (5xx / server_error) are NOT deterministic +// credential rejections — they must stay UNTYPED so a probe caller treats them +// as upstream noise and stays silent (and retryers can back off). +func TestFetchTAT_ServerError_Untyped(t *testing.T) { + rt := &stubRoundTripper{respCode: 500, respBody: `{"code":20050,"error":"server_error","error_description":"please retry"}`} + hc := &http.Client{Transport: rt} + + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error for server_error") + } + if errs.IsTyped(err) { + t.Errorf("server_error must be UNTYPED (transient), got typed %T %v", err, err) + } +} + +// Rate-limiting is transient, not a deterministic credential rejection — an HTTP +// 429 (even with a parseable OAuth body) and the OAuth slow_down error must both +// stay UNTYPED so a rate-limited probe stays silent and retryers can back off. +func TestFetchTAT_RateLimit_Untyped(t *testing.T) { + cases := []struct { + name string + code int + body string + }{ + {"http 429", 429, `{"code":99991400,"error":"too_many_requests","error_description":"rate limit exceeded"}`}, + {"oauth slow_down", 200, `{"error":"slow_down","error_description":"polling too fast"}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := &stubRoundTripper{respCode: tc.code, respBody: tc.body} + hc := &http.Client{Transport: rt} + + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error for rate-limit") + } + if errs.IsTyped(err) { + t.Errorf("rate-limit must be UNTYPED (transient), got typed %T %v", err, err) + } + }) + } +} + +// Non-2xx HTTP with a non-JSON body is ambiguous (not a structured OAuth +// rejection) — it must stay UNTYPED so a probe caller treats it as upstream +// noise and stays silent. func TestFetchTAT_HTTPNon200_Untyped(t *testing.T) { for _, code := range []int{401, 403, 500, 503} { rt := &stubRoundTripper{respCode: code, respBody: `whatever`} @@ -182,12 +254,12 @@ func TestFetchTAT_BrandRouting(t *testing.T) { brand core.LarkBrand wantURL string }{ - {core.BrandFeishu, "https://open.feishu.cn/open-apis/auth/v3/tenant_access_token/internal"}, - {core.BrandLark, "https://open.larksuite.com/open-apis/auth/v3/tenant_access_token/internal"}, + {core.BrandFeishu, "https://accounts.feishu.cn/oauth/v3/token"}, + {core.BrandLark, "https://accounts.larksuite.com/oauth/v3/token"}, } for _, tc := range tests { t.Run(string(tc.brand), func(t *testing.T) { - rt := &stubRoundTripper{respCode: 200, respBody: `{"code":0,"tenant_access_token":"t"}`} + rt := &stubRoundTripper{respCode: 200, respBody: `{"code":0,"access_token":"t","token_type":"Bearer"}`} hc := &http.Client{Transport: rt} if _, err := FetchTAT(context.Background(), hc, tc.brand, "a", "b"); err != nil { t.Fatal(err) diff --git a/internal/errclass/codemeta.go b/internal/errclass/codemeta.go index 7c41c875..d0b1d764 100644 --- a/internal/errclass/codemeta.go +++ b/internal/errclass/codemeta.go @@ -65,7 +65,7 @@ var codeMeta = map[int]CodeMeta{ // CategoryConfig 99991543: {Category: errs.CategoryConfig, Subtype: errs.SubtypeInvalidClient}, // RFC 6749 §5.2 — app_id / app_secret incorrect (Open API) - 10014: {Category: errs.CategoryConfig, Subtype: errs.SubtypeInvalidClient}, // TAT endpoint — "app secret invalid" (TAT-mint variant of 99991543) + 10014: {Category: errs.CategoryConfig, Subtype: errs.SubtypeInvalidClient}, // legacy TAT endpoint — "app secret invalid" (pre-v3 variant of 99991543; CLI now reports invalid_client) // CategoryPolicy 21000: {Category: errs.CategoryPolicy, Subtype: errs.SubtypeChallengeRequired}, diff --git a/internal/output/lark_errors.go b/internal/output/lark_errors.go index 892c2817..137c342f 100644 --- a/internal/output/lark_errors.go +++ b/internal/output/lark_errors.go @@ -35,9 +35,12 @@ const ( LarkErrAppNotInUse = 99991662 // app is disabled in this tenant LarkErrAppUnauthorized = 99991673 // app status unavailable; check installation - // TAT-endpoint variant of the "wrong app credentials" condition. - // /open-apis/auth/v3/tenant_access_token/internal returns code 10014 - // ("app secret invalid") instead of 99991543 when the secret is wrong. + // "Wrong app credentials" code from the LEGACY TAT endpoint + // (/open-apis/auth/v3/tenant_access_token/internal returns 10014, "app secret + // invalid", instead of 99991543). Since the OAuth v3 migration the CLI mints + // TAT via accounts/oauth/v3/token and reports this as the OAuth invalid_client + // error, so it no longer emits 10014 itself; the constant + codemeta mapping + // are retained as a defensive fallback should 10014 still arrive. LarkErrTATInvalidSecret = 10014 // Rate limit.