From e6bc292575899cffb2c2aaa957d1e8f84879fbe8 Mon Sep 17 00:00:00 2001 From: liangshuo-1 Date: Tue, 19 May 2026 15:50:40 +0800 Subject: [PATCH] fix(identitydiag): harden verify path and tighten status semantics (#961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(identitydiag): harden verify path and tighten status semantics Follow-ups to #957: - bound bot/user verify calls with a 10s timeout (mirrors the doctor endpoint probe) so a hanging server cannot wedge `auth status --verify` or `doctor` - return StatusNotConfigured (not StatusMissing) when the user-identity path is blocked by missing app config, matching the bot side - surface the `{code, msg}` envelope on bot-info HTTP 4xx responses so callers see why bot auth was rejected, not just the bare HTTP code - introduce identity{User,Bot,None} constants in cmd/auth/status.go and use the exported StatusMessage() in the human-readable note instead of raw status codes like "not_configured" - collapse the duplicated verify-failed identity construction in the user path into a local helper - cover the new failure paths with unit tests (HTTP 4xx with envelope, business error code, user server-rejected, expired user token, strict-mode user-only, missing app config for user) Change-Id: I581348a65f15b1452a6f48a3e3245d09257314ac * fix(identitydiag): decode bot/v3/info from "bot" field, not "data" `/open-apis/bot/v3/info` returns `{code, msg, bot: {...}}` — the bot payload is under `bot`, not `data` as the newer Lark API convention would suggest. The decoder was reading from a non-existent `data` field, so `envelope.Data.OpenID` was always empty and every successful verify was reported as `Bot identity: verify failed: open_id is empty`. The pre-existing test mocks used `{"data": {...}}` matching the buggy decoder, so unit tests passed while production reads of every Lark account failed verification. Fix: - change the JSON tag on the envelope from `json:"data"` to `json:"bot"` - update mocks in identitydiag and cmd/auth/status tests to emit `bot` Verified locally: `lark-cli doctor` now reports `bot_identity: pass` for both a normal account and a bot-only profile, restoring the behavior that #957 set out to deliver. Change-Id: Ib26dfdd5a0cc37d2d62537ae2bf5e854e67cb83c * fix(shortcuts/common): decode bot/v3/info from "bot" field, not "data" Same schema bug as the one fixed in identitydiag — `RuntimeContext. fetchBotInfo` reads from a non-existent "data" key, so every successful call would report "open_id is empty" once a caller starts depending on it. There are no production callers of `RuntimeContext.BotInfo()` yet (only tests + the `TestNewRuntimeContextWithBotInfo` helper), so this bug is dormant — but the pre-existing tests pass with the same wrong schema in their mocks, so the first real consumer would silently break. Fix: tag `json:"data"` → `json:"bot"` plus aligning the four mock fixtures in runner_botinfo_test.go. The Go field name `Data` is kept to minimize the diff; only the JSON contract is corrected. Change-Id: I11e1e871603e5349f8df29b1d58e35d07b628dfd --- cmd/auth/status.go | 18 ++- cmd/auth/status_test.go | 2 +- internal/identitydiag/diagnostics.go | 79 +++++---- internal/identitydiag/diagnostics_test.go | 186 +++++++++++++++++++++- shortcuts/common/runner.go | 4 +- shortcuts/common/runner_botinfo_test.go | 8 +- 6 files changed, 248 insertions(+), 49 deletions(-) diff --git a/cmd/auth/status.go b/cmd/auth/status.go index 914fd17a..20a8d479 100644 --- a/cmd/auth/status.go +++ b/cmd/auth/status.go @@ -69,14 +69,20 @@ func authStatusRun(opts *StatusOptions) error { return nil } +const ( + identityUser = "user" + identityBot = "bot" + identityNone = "none" +) + func effectiveIdentity(d identitydiag.Result) string { switch { case d.User.Available: - return "user" + return identityUser case d.Bot.Available: - return "bot" + return identityBot default: - return "none" + return identityNone } } @@ -105,14 +111,14 @@ func addLegacyUserFields(result map[string]interface{}, user identitydiag.Identi func addEffectiveVerification(result map[string]interface{}, d identitydiag.Result) { switch result["identity"] { - case "user": + case identityUser: if d.User.Verified != nil { result["verified"] = *d.User.Verified if !*d.User.Verified { result["verifyError"] = d.User.Message } } - case "bot": + case identityBot: if d.Bot.Verified != nil { result["verified"] = *d.Bot.Verified if !*d.Bot.Verified { @@ -125,7 +131,7 @@ func addEffectiveVerification(result map[string]interface{}, d identitydiag.Resu func addStatusNote(result map[string]interface{}, d identitydiag.Result) { switch { case !d.User.Available && d.Bot.Available: - result["note"] = "User identity is " + d.User.Status + "; bot identity is ready for bot/tenant API calls. Run `lark-cli auth login` to enable user identity." + result["note"] = "User identity is " + identitydiag.StatusMessage(d.User.Status) + "; bot identity is ready for bot/tenant API calls. Run `lark-cli auth login` to enable user identity." case d.User.Status == identitydiag.StatusNeedsRefresh: result["note"] = "User identity needs refresh and will be refreshed automatically on the next user API call." case !d.User.Available && !d.Bot.Available: diff --git a/cmd/auth/status_test.go b/cmd/auth/status_test.go index 28ea264c..7bf0608c 100644 --- a/cmd/auth/status_test.go +++ b/cmd/auth/status_test.go @@ -47,7 +47,7 @@ func TestAuthStatusRun_VerifyReportsBotIdentity(t *testing.T) { Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "ou_bot", "app_name": "diagnostic bot", }, diff --git a/internal/identitydiag/diagnostics.go b/internal/identitydiag/diagnostics.go index c6f68b17..f8eb648d 100644 --- a/internal/identitydiag/diagnostics.go +++ b/internal/identitydiag/diagnostics.go @@ -27,6 +27,11 @@ const ( StatusVerifyFailed = "verify_failed" ) +// verifyTimeout bounds each network call made during --verify so that a +// hanging server cannot wedge `auth status --verify` or `doctor`. Mirrors +// the 10s timeout used by the doctor endpoint probe. +const verifyTimeout = 10 * time.Second + // Result describes the independently usable bot and user identities. type Result struct { Bot Identity `json:"bot"` @@ -104,7 +109,7 @@ func diagnoseBot(ctx context.Context, f *cmdutil.Factory, cfg *core.CliConfig, v return Identity{ Status: status, Verified: boolPtr(false), - Message: "Bot identity: " + statusMessage(status) + ": " + err.Error(), + Message: "Bot identity: " + StatusMessage(status) + ": " + err.Error(), Hint: "check app credentials or the active credential provider", } } @@ -128,8 +133,8 @@ func diagnoseBot(ctx context.Context, f *cmdutil.Factory, cfg *core.CliConfig, v func diagnoseUser(ctx context.Context, f *cmdutil.Factory, cfg *core.CliConfig, verify bool) Identity { if cfg == nil || cfg.AppID == "" { return Identity{ - Status: StatusMissing, - Message: "User identity: missing (missing app config)", + Status: StatusNotConfigured, + Message: "User identity: not configured (missing app config)", Hint: "run: lark-cli config --help", } } @@ -174,38 +179,33 @@ func diagnoseUser(ctx context.Context, f *cmdutil.Factory, cfg *core.CliConfig, return id } - httpClient, err := f.HttpClient() - if err != nil { + markVerifyFailed := func(reason, hint string) Identity { id.Status = StatusVerifyFailed id.Available = false id.Verified = boolPtr(false) - id.Message = "User identity: verify failed: create HTTP client: " + err.Error() + id.Message = "User identity: verify failed: " + reason + if hint != "" { + id.Hint = hint + } return id } + + httpClient, err := f.HttpClient() + if err != nil { + return markVerifyFailed("create HTTP client: "+err.Error(), "") + } token, err := larkauth.GetValidAccessToken(httpClient, larkauth.NewUATCallOptions(cfg, f.IOStreams.ErrOut)) if err != nil { - id.Status = StatusVerifyFailed - id.Available = false - id.Verified = boolPtr(false) - id.Message = "User identity: verify failed: token unusable: " + err.Error() - id.Hint = "run: lark-cli auth login --help" - return id + return markVerifyFailed("token unusable: "+err.Error(), "run: lark-cli auth login --help") } sdk, err := f.LarkClient() if err != nil { - id.Status = StatusVerifyFailed - id.Available = false - id.Verified = boolPtr(false) - id.Message = "User identity: verify failed: SDK init failed: " + err.Error() - return id + return markVerifyFailed("SDK init failed: "+err.Error(), "") } - if err := larkauth.VerifyUserToken(ctx, sdk, token); err != nil { - id.Status = StatusVerifyFailed - id.Available = false - id.Verified = boolPtr(false) - id.Message = "User identity: verify failed: server rejected token: " + err.Error() - id.Hint = "run: lark-cli auth login --help" - return id + verifyCtx, cancel := context.WithTimeout(ctx, verifyTimeout) + defer cancel() + if err := larkauth.VerifyUserToken(verifyCtx, sdk, token); err != nil { + return markVerifyFailed("server rejected token: "+err.Error(), "run: lark-cli auth login --help") } id.Verified = boolPtr(true) @@ -241,6 +241,8 @@ func fetchBotInfo(ctx context.Context, f *cmdutil.Factory, cfg *core.CliConfig, if err != nil { return nil, fmt.Errorf("create HTTP client: %w", err) } + ctx, cancel := context.WithTimeout(ctx, verifyTimeout) + defer cancel() url := strings.TrimRight(core.ResolveEndpoints(cfg.Brand).Open, "/") + "/open-apis/bot/v3/info" req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { @@ -253,24 +255,31 @@ func fetchBotInfo(ctx context.Context, f *cmdutil.Factory, cfg *core.CliConfig, return nil, err } defer resp.Body.Close() - if resp.StatusCode >= 400 { - return nil, fmt.Errorf("HTTP %d", resp.StatusCode) - } - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } + body, _ := io.ReadAll(resp.Body) + // /open-apis/bot/v3/info returns `{code, msg, bot: {...}}` — the bot + // payload is under "bot", not "data" as the newer Lark API convention. var envelope struct { Code int `json:"code"` Msg string `json:"msg"` Data struct { OpenID string `json:"open_id"` AppName string `json:"app_name"` - } `json:"data"` + } `json:"bot"` } - if err := json.Unmarshal(body, &envelope); err != nil { - return nil, fmt.Errorf("parse response: %w", err) + parseErr := json.Unmarshal(body, &envelope) + + if resp.StatusCode >= 400 { + // Lark error responses are usually `{code, msg}` envelopes even on + // non-2xx — surface them when present so callers see why bot auth + // was rejected, not just the bare HTTP code. + if parseErr == nil && envelope.Code != 0 { + return nil, fmt.Errorf("HTTP %d: [%d] %s", resp.StatusCode, envelope.Code, envelope.Msg) + } + return nil, fmt.Errorf("HTTP %d", resp.StatusCode) + } + if parseErr != nil { + return nil, fmt.Errorf("parse response: %w", parseErr) } if envelope.Code != 0 { return nil, fmt.Errorf("[%d] %s", envelope.Code, envelope.Msg) @@ -296,7 +305,7 @@ func formatMillis(ms int64) string { return time.UnixMilli(ms).Format(time.RFC3339) } -func statusMessage(status string) string { +func StatusMessage(status string) string { switch status { case StatusNotConfigured: return "not configured" diff --git a/internal/identitydiag/diagnostics_test.go b/internal/identitydiag/diagnostics_test.go index 3d42da85..6d288e3b 100644 --- a/internal/identitydiag/diagnostics_test.go +++ b/internal/identitydiag/diagnostics_test.go @@ -6,6 +6,7 @@ package identitydiag import ( "context" "net/http" + "strings" "testing" "time" @@ -48,7 +49,7 @@ func TestDiagnose_VerifyBotIdentity(t *testing.T) { Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "ou_bot", "app_name": "diagnostic bot", }, @@ -104,7 +105,7 @@ func TestDiagnose_VerifyUserIdentity(t *testing.T) { Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "ou_bot", "app_name": "diagnostic bot", }, @@ -131,6 +132,187 @@ func TestDiagnose_VerifyUserIdentity(t *testing.T) { } } +func TestDiagnose_VerifyBotIdentity_HTTPErrorSurfacesEnvelope(t *testing.T) { + cfg := &core.CliConfig{AppID: "test-app", AppSecret: "secret", Brand: core.BrandFeishu} + f, _, _, reg := cmdutil.TestFactory(t, cfg) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/bot/v3/info", + Status: http.StatusUnauthorized, + Body: map[string]interface{}{ + "code": 99991663, + "msg": "app ticket invalid", + }, + }) + + got := Diagnose(context.Background(), f, cfg, true) + if got.Bot.Status != StatusVerifyFailed || got.Bot.Available { + t.Fatalf("bot = %#v, want verify_failed and unavailable", got.Bot) + } + if got.Bot.Verified == nil || *got.Bot.Verified { + t.Fatalf("bot verified = %v, want false", got.Bot.Verified) + } + if !strings.Contains(got.Bot.Message, "401") || !strings.Contains(got.Bot.Message, "99991663") { + t.Fatalf("bot message = %q, want both HTTP code and envelope code", got.Bot.Message) + } +} + +func TestDiagnose_VerifyBotIdentity_BusinessErrorCode(t *testing.T) { + cfg := &core.CliConfig{AppID: "test-app", AppSecret: "secret", Brand: core.BrandFeishu} + f, _, _, reg := cmdutil.TestFactory(t, cfg) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/bot/v3/info", + Body: map[string]interface{}{ + "code": 10013, + "msg": "scope not granted", + }, + }) + + got := Diagnose(context.Background(), f, cfg, true) + if got.Bot.Status != StatusVerifyFailed || got.Bot.Available { + t.Fatalf("bot = %#v, want verify_failed and unavailable", got.Bot) + } + if !strings.Contains(got.Bot.Message, "10013") || !strings.Contains(got.Bot.Message, "scope not granted") { + t.Fatalf("bot message = %q, want envelope code/msg", got.Bot.Message) + } +} + +func TestDiagnose_VerifyUserIdentity_ServerRejects(t *testing.T) { + keyring.MockInit() + t.Setenv("HOME", t.TempDir()) + t.Setenv("LARKSUITE_CLI_DATA_DIR", t.TempDir()) + + cfg := &core.CliConfig{ + AppID: "test-app-reject", + AppSecret: "secret", + Brand: core.BrandFeishu, + UserOpenId: "ou_user", + UserName: "tester", + } + now := time.Now() + if err := larkauth.SetStoredToken(&larkauth.StoredUAToken{ + AppId: cfg.AppID, + UserOpenId: cfg.UserOpenId, + AccessToken: "user-access-token", + RefreshToken: "refresh-token", + ExpiresAt: now.Add(time.Hour).UnixMilli(), + RefreshExpiresAt: now.Add(24 * time.Hour).UnixMilli(), + GrantedAt: now.Add(-time.Hour).UnixMilli(), + Scope: "offline_access", + }); err != nil { + t.Fatalf("SetStoredToken() error = %v", err) + } + + f, _, _, reg := cmdutil.TestFactory(t, cfg) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/bot/v3/info", + Body: map[string]interface{}{ + "code": 0, + "bot": map[string]interface{}{"open_id": "ou_bot", "app_name": "bot"}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: larkauth.PathUserInfoV1, + Body: map[string]interface{}{ + "code": 99991661, + "msg": "access token invalid", + }, + }) + + got := Diagnose(context.Background(), f, cfg, true) + if got.User.Status != StatusVerifyFailed || got.User.Available { + t.Fatalf("user = %#v, want verify_failed and unavailable", got.User) + } + if got.User.Verified == nil || *got.User.Verified { + t.Fatalf("user verified = %v, want false", got.User.Verified) + } + if !strings.Contains(got.User.Message, "server rejected token") { + t.Fatalf("user message = %q, want 'server rejected token'", got.User.Message) + } +} + +func TestDiagnose_UserIdentityExpired(t *testing.T) { + keyring.MockInit() + t.Setenv("HOME", t.TempDir()) + t.Setenv("LARKSUITE_CLI_DATA_DIR", t.TempDir()) + + cfg := &core.CliConfig{ + AppID: "test-app-expired", + AppSecret: "secret", + Brand: core.BrandFeishu, + UserOpenId: "ou_expired", + UserName: "tester", + } + now := time.Now() + if err := larkauth.SetStoredToken(&larkauth.StoredUAToken{ + AppId: cfg.AppID, + UserOpenId: cfg.UserOpenId, + AccessToken: "user-access-token", + RefreshToken: "refresh-token", + ExpiresAt: now.Add(-time.Hour).UnixMilli(), + RefreshExpiresAt: now.Add(-time.Minute).UnixMilli(), + GrantedAt: now.Add(-24 * time.Hour).UnixMilli(), + Scope: "offline_access", + }); err != nil { + t.Fatalf("SetStoredToken() error = %v", err) + } + + f, _, _, _ := cmdutil.TestFactory(t, cfg) + got := Diagnose(context.Background(), f, cfg, false) + if got.User.Status != StatusMissing || got.User.Available { + t.Fatalf("user = %#v, want missing and unavailable", got.User) + } + if got.User.Hint == "" { + t.Fatalf("user hint is empty, want re-login hint") + } +} + +func TestDiagnose_BotIdentityStrictUserOnly(t *testing.T) { + // SupportedIdentities = SupportsUser (1) only — bot path should be + // reported as not_configured even though an app secret is present. + cfg := &core.CliConfig{ + AppID: "test-app", + AppSecret: "secret", + Brand: core.BrandFeishu, + SupportedIdentities: 1, + } + f, _, _, _ := cmdutil.TestFactory(t, cfg) + + got := Diagnose(context.Background(), f, cfg, false) + if got.Bot.Status != StatusNotConfigured || got.Bot.Available { + t.Fatalf("bot = %#v, want not_configured and unavailable", got.Bot) + } +} + +func TestDiagnose_UserIdentityMissingAppConfig(t *testing.T) { + cfg := &core.CliConfig{Brand: core.BrandFeishu} + f, _, _, _ := cmdutil.TestFactory(t, cfg) + + got := Diagnose(context.Background(), f, cfg, false) + if got.User.Status != StatusNotConfigured || got.User.Available { + t.Fatalf("user = %#v, want not_configured and unavailable", got.User) + } +} + +func TestStatusMessage(t *testing.T) { + cases := map[string]string{ + StatusReady: StatusReady, + StatusNotConfigured: "not configured", + StatusVerifyFailed: "verify failed", + StatusNeedsRefresh: "needs refresh", + StatusMissing: "missing", + "unknown": "unknown", + } + for in, want := range cases { + if got := StatusMessage(in); got != want { + t.Errorf("StatusMessage(%q) = %q, want %q", in, got, want) + } + } +} + func TestDiagnose_UserIdentityNeedsRefresh(t *testing.T) { keyring.MockInit() t.Setenv("HOME", t.TempDir()) diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index 7e4c8ece..d6f4c1a5 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -103,13 +103,15 @@ func (ctx *RuntimeContext) fetchBotInfo() (*BotInfo, error) { if resp.StatusCode >= 400 { return nil, fmt.Errorf("fetch bot info: HTTP %d", resp.StatusCode) } + // /open-apis/bot/v3/info returns `{code, msg, bot: {...}}` — the bot + // payload is under "bot", not "data" as the newer Lark API convention. var envelope struct { Code int `json:"code"` Msg string `json:"msg"` Data struct { OpenID string `json:"open_id"` AppName string `json:"app_name"` - } `json:"data"` + } `json:"bot"` } if err := json.Unmarshal(resp.RawBody, &envelope); err != nil { return nil, fmt.Errorf("fetch bot info: unmarshal: %w", err) diff --git a/shortcuts/common/runner_botinfo_test.go b/shortcuts/common/runner_botinfo_test.go index 0ca121f7..9a4247c0 100644 --- a/shortcuts/common/runner_botinfo_test.go +++ b/shortcuts/common/runner_botinfo_test.go @@ -57,7 +57,7 @@ func TestFetchBotInfo_Success(t *testing.T) { URL: "/open-apis/bot/v3/info", Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "ou_bot_abc123", "app_name": "TestBot", }, @@ -86,7 +86,7 @@ func TestFetchBotInfo_ShortcutHeaders(t *testing.T) { URL: "/open-apis/bot/v3/info", Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "ou_bot_header", "app_name": "HeaderBot", }, @@ -119,7 +119,7 @@ func TestFetchBotInfo_OnceSemantics(t *testing.T) { URL: "/open-apis/bot/v3/info", Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "ou_bot_once", "app_name": "OnceBot", }, @@ -183,7 +183,7 @@ func TestFetchBotInfo_EmptyOpenID(t *testing.T) { URL: "/open-apis/bot/v3/info", Body: map[string]interface{}{ "code": 0, "msg": "ok", - "data": map[string]interface{}{ + "bot": map[string]interface{}{ "open_id": "", "app_name": "EmptyBot", },