fix(identitydiag): harden verify path and tighten status semantics (#961)

* 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
This commit is contained in:
liangshuo-1
2026-05-19 15:50:40 +08:00
committed by GitHub
parent 4aa61db8b2
commit e6bc292575
6 changed files with 248 additions and 49 deletions

View File

@@ -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:

View File

@@ -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",
},

View File

@@ -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"

View File

@@ -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())

View File

@@ -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)

View File

@@ -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",
},