From fbc10e752cf0c3c0d9944a7e9ffc8a94f91c8445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=A2=81=E7=A1=95?= Date: Sun, 5 Apr 2026 23:34:15 +0800 Subject: [PATCH] fix: tighten credential resolution and profile flows Change-Id: I83f6d424540eab9b1708944b9b6e26e8477cc60d --- cmd/config/config_test.go | 54 ++++++ cmd/config/init.go | 89 +++++++--- cmd/profile/add.go | 5 + cmd/profile/profile_test.go | 107 +++++++++++ cmd/profile/remove.go | 15 +- extension/credential/env/env.go | 37 +++- extension/credential/env/env_test.go | 64 ++++++- internal/client/client.go | 31 +++- internal/client/client_test.go | 85 +++++++++ internal/cmdutil/factory.go | 9 +- internal/cmdutil/factory_default.go | 13 +- internal/cmdutil/factory_default_test.go | 38 ++++ internal/cmdutil/factory_test.go | 19 +- internal/credential/credential_provider.go | 167 ++++++++++++++---- .../credential/credential_provider_test.go | 112 +++++++++++- internal/credential/types.go | 17 +- shortcuts/common/runner.go | 19 +- shortcuts/common/runner_scope_test.go | 35 ++++ 18 files changed, 810 insertions(+), 106 deletions(-) create mode 100644 cmd/profile/profile_test.go diff --git a/cmd/config/config_test.go b/cmd/config/config_test.go index 65642781f..bd939c5b9 100644 --- a/cmd/config/config_test.go +++ b/cmd/config/config_test.go @@ -10,8 +10,15 @@ import ( "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/keychain" ) +type noopConfigKeychain struct{} + +func (n *noopConfigKeychain) Get(service, account string) (string, error) { return "", nil } +func (n *noopConfigKeychain) Set(service, account, value string) error { return nil } +func (n *noopConfigKeychain) Remove(service, account string) error { return nil } + func TestConfigInitCmd_FlagParsing(t *testing.T) { f, _, _, _ := cmdutil.TestFactory(t, nil) f.IOStreams.In = strings.NewReader("secret123\n") @@ -157,3 +164,50 @@ func TestConfigRemoveCmd_FlagParsing(t *testing.T) { t.Fatal("expected factory to be preserved in options") } } + +func TestSaveAsProfile_RejectsProfileNameCollisionWithExistingAppID(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + + existing := &core.MultiAppConfig{ + Apps: []core.AppConfig{ + { + Name: "prod", + AppId: "cli_prod", + AppSecret: core.PlainSecret("secret"), + Brand: core.BrandFeishu, + }, + }, + } + + err := saveAsProfile(existing, keychain.KeychainAccess(&noopConfigKeychain{}), "cli_prod", "app-new", core.PlainSecret("new-secret"), core.BrandLark, "en") + if err == nil { + t.Fatal("expected conflict error") + } + if !strings.Contains(err.Error(), "conflicts with existing appId") { + t.Fatalf("error = %v, want conflict with existing appId", err) + } +} + +func TestUpdateExistingProfileWithoutSecret_RejectsAppIDChange(t *testing.T) { + multi := &core.MultiAppConfig{ + CurrentApp: "prod", + Apps: []core.AppConfig{ + { + Name: "prod", + AppId: "app-old", + AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:app-old"}}, + Brand: core.BrandFeishu, + Lang: "zh", + Users: []core.AppUser{{UserOpenId: "ou_1", UserName: "User"}}, + }, + }, + } + + err := updateExistingProfileWithoutSecret(multi, "", "app-new", core.BrandLark, "en") + if err == nil { + t.Fatal("expected error when changing app ID without a new secret") + } + if !strings.Contains(err.Error(), "App Secret") { + t.Fatalf("error = %v, want mention of App Secret", err) + } +} diff --git a/cmd/config/init.go b/cmd/config/init.go index 87d5ee467..3a3a7a84a 100644 --- a/cmd/config/init.go +++ b/cmd/config/init.go @@ -6,6 +6,7 @@ package config import ( "bufio" "context" + "errors" "fmt" "io" "strings" @@ -117,7 +118,7 @@ func saveAsProfile(existing *core.MultiAppConfig, kc keychain.KeychainAccess, pr multi = &core.MultiAppConfig{} } - if idx := multi.FindAppIndex(profileName); idx >= 0 { + if idx := findProfileIndexByName(multi, profileName); idx >= 0 { // Clean up old keychain secret and user tokens if AppId changed if multi.Apps[idx].AppId != appId { core.RemoveSecretStore(multi.Apps[idx].AppSecret, kc) @@ -132,6 +133,9 @@ func saveAsProfile(existing *core.MultiAppConfig, kc keychain.KeychainAccess, pr multi.Apps[idx].Brand = brand multi.Apps[idx].Lang = lang } else { + if findAppIndexByAppID(multi, profileName) >= 0 { + return fmt.Errorf("profile name %q conflicts with existing appId", profileName) + } // Append new profile multi.Apps = append(multi.Apps, core.AppConfig{ Name: profileName, @@ -145,6 +149,59 @@ func saveAsProfile(existing *core.MultiAppConfig, kc keychain.KeychainAccess, pr return core.SaveMultiAppConfig(multi) } +func findProfileIndexByName(multi *core.MultiAppConfig, profileName string) int { + if multi == nil { + return -1 + } + for i := range multi.Apps { + if multi.Apps[i].Name == profileName { + return i + } + } + return -1 +} + +func findAppIndexByAppID(multi *core.MultiAppConfig, appID string) int { + if multi == nil { + return -1 + } + for i := range multi.Apps { + if multi.Apps[i].AppId == appID { + return i + } + } + return -1 +} + +func updateExistingProfileWithoutSecret(existing *core.MultiAppConfig, profileName, appID string, brand core.LarkBrand, lang string) error { + if existing == nil { + return output.ErrValidation("App Secret cannot be empty for new configuration") + } + + var app *core.AppConfig + if profileName != "" { + if idx := findProfileIndexByName(existing, profileName); idx >= 0 { + app = &existing.Apps[idx] + } else { + return output.ErrValidation("App Secret cannot be empty for new profile") + } + } else { + app = existing.CurrentAppConfig("") + if app == nil { + return output.ErrValidation("App Secret cannot be empty for new configuration") + } + } + + if app.AppId != appID { + return output.ErrValidation("App Secret cannot be empty when changing App ID") + } + + app.AppId = appID + app.Brand = brand + app.Lang = lang + return core.SaveMultiAppConfig(existing) +} + func configInitRun(opts *ConfigInitOptions) error { f := opts.Factory @@ -254,32 +311,12 @@ func configInitRun(opts *ConfigInitOptions) error { } } else if result.Mode == "existing" && result.AppID != "" { // Existing app with unchanged secret — update app ID and brand only - if opts.ProfileName != "" && existing != nil { - // Profile mode: update named profile in-place - if idx := existing.FindAppIndex(opts.ProfileName); idx >= 0 { - existing.Apps[idx].AppId = result.AppID - existing.Apps[idx].Brand = result.Brand - existing.Apps[idx].Lang = opts.Lang - } else { - return output.ErrValidation("App Secret cannot be empty for new profile") + if err := updateExistingProfileWithoutSecret(existing, opts.ProfileName, result.AppID, result.Brand, opts.Lang); err != nil { + var exitErr *output.ExitError + if errors.As(err, &exitErr) { + return err } - if err := core.SaveMultiAppConfig(existing); err != nil { - return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) - } - } else if existing != nil { - app := existing.CurrentAppConfig("") - if app != nil { - app.AppId = result.AppID - app.Brand = result.Brand - app.Lang = opts.Lang - if err := core.SaveMultiAppConfig(existing); err != nil { - return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) - } - } else { - return output.ErrValidation("App Secret cannot be empty for new configuration") - } - } else { - return output.ErrValidation("App Secret cannot be empty for new configuration") + return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) } } else { return output.ErrValidation("App ID and App Secret cannot be empty") diff --git a/cmd/profile/add.go b/cmd/profile/add.go index 4679192fb..1ad324b70 100644 --- a/cmd/profile/add.go +++ b/cmd/profile/add.go @@ -5,7 +5,9 @@ package profile import ( "bufio" + "errors" "fmt" + "os" "strings" "github.com/spf13/cobra" @@ -71,6 +73,9 @@ func profileAddRun(f *cmdutil.Factory, name, appID string, appSecretStdin bool, // Load or create config multi, err := core.LoadMultiAppConfig() if err != nil { + if !errors.Is(err, os.ErrNotExist) { + return output.Errorf(output.ExitInternal, "internal", "failed to load config: %v", err) + } multi = &core.MultiAppConfig{} } diff --git a/cmd/profile/profile_test.go b/cmd/profile/profile_test.go new file mode 100644 index 000000000..9cb485639 --- /dev/null +++ b/cmd/profile/profile_test.go @@ -0,0 +1,107 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package profile + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" +) + +func setupProfileConfigDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + return dir +} + +func TestProfileAddRun_InvalidExistingConfigReturnsError(t *testing.T) { + dir := setupProfileConfigDir(t) + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte("{invalid json"), 0600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + f, _, _, _ := cmdutil.TestFactory(t, nil) + f.IOStreams.In = strings.NewReader("secret\n") + + err := profileAddRun(f, "test", "app-test", true, "feishu", "zh", false) + if err == nil { + t.Fatal("expected error for invalid existing config") + } + if !strings.Contains(err.Error(), "failed to load config") { + t.Fatalf("error = %v, want failed to load config", err) + } +} + +func TestProfileAddRun_UseAfterUpdatesCurrentAndPrevious(t *testing.T) { + setupProfileConfigDir(t) + multi := &core.MultiAppConfig{ + CurrentApp: "default", + Apps: []core.AppConfig{ + {Name: "default", AppId: "app-default", AppSecret: core.PlainSecret("secret-default"), Brand: core.BrandFeishu}, + }, + } + if err := core.SaveMultiAppConfig(multi); err != nil { + t.Fatalf("SaveMultiAppConfig() error = %v", err) + } + + f, _, _, _ := cmdutil.TestFactory(t, nil) + f.IOStreams.In = strings.NewReader("secret-new\n") + + if err := profileAddRun(f, "target", "app-target", true, "lark", "en", true); err != nil { + t.Fatalf("profileAddRun() error = %v", err) + } + + saved, err := core.LoadMultiAppConfig() + if err != nil { + t.Fatalf("LoadMultiAppConfig() error = %v", err) + } + if saved.CurrentApp != "target" { + t.Fatalf("CurrentApp = %q, want %q", saved.CurrentApp, "target") + } + if saved.PreviousApp != "default" { + t.Fatalf("PreviousApp = %q, want %q", saved.PreviousApp, "default") + } + if len(saved.Apps) != 2 { + t.Fatalf("len(Apps) = %d, want 2", len(saved.Apps)) + } +} + +func TestProfileRemoveRun_RemovesCurrentProfileAndSwitchesToFirstRemaining(t *testing.T) { + setupProfileConfigDir(t) + multi := &core.MultiAppConfig{ + CurrentApp: "target", + PreviousApp: "default", + Apps: []core.AppConfig{ + {Name: "default", AppId: "app-default", AppSecret: core.PlainSecret("secret-default"), Brand: core.BrandFeishu}, + {Name: "target", AppId: "app-target", AppSecret: core.PlainSecret("secret-target"), Brand: core.BrandLark}, + }, + } + if err := core.SaveMultiAppConfig(multi); err != nil { + t.Fatalf("SaveMultiAppConfig() error = %v", err) + } + + f, _, _, _ := cmdutil.TestFactory(t, nil) + if err := profileRemoveRun(f, "target"); err != nil { + t.Fatalf("profileRemoveRun() error = %v", err) + } + + saved, err := core.LoadMultiAppConfig() + if err != nil { + t.Fatalf("LoadMultiAppConfig() error = %v", err) + } + if saved.CurrentApp != "default" { + t.Fatalf("CurrentApp = %q, want %q", saved.CurrentApp, "default") + } + if saved.PreviousApp != "default" { + t.Fatalf("PreviousApp = %q, want %q", saved.PreviousApp, "default") + } + if len(saved.Apps) != 1 || saved.Apps[0].ProfileName() != "default" { + t.Fatalf("remaining apps = %#v, want only default", saved.Apps) + } +} diff --git a/cmd/profile/remove.go b/cmd/profile/remove.go index ea6c4595f..5255b8967 100644 --- a/cmd/profile/remove.go +++ b/cmd/profile/remove.go @@ -48,12 +48,9 @@ func profileRemoveRun(f *cmdutil.Factory, name string) error { app := &multi.Apps[idx] removedName := app.ProfileName() - - // Cleanup keychain: app secret + user tokens - core.RemoveSecretStore(app.AppSecret, f.Keychain) - for _, user := range app.Users { - larkauth.RemoveStoredToken(app.AppId, user.UserOpenId) - } + appId := app.AppId + appSecret := app.AppSecret + users := app.Users // Remove from slice multi.Apps = append(multi.Apps[:idx], multi.Apps[idx+1:]...) @@ -70,6 +67,12 @@ func profileRemoveRun(f *cmdutil.Factory, name string) error { return fmt.Errorf("failed to save config: %w", err) } + // Best-effort credential cleanup after config commit + core.RemoveSecretStore(appSecret, f.Keychain) + for _, user := range users { + larkauth.RemoveStoredToken(appId, user.UserOpenId) + } + output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf("Profile %q removed", removedName)) return nil } diff --git a/extension/credential/env/env.go b/extension/credential/env/env.go index 514c127e6..4273b1297 100644 --- a/extension/credential/env/env.go +++ b/extension/credential/env/env.go @@ -5,6 +5,7 @@ package env import ( "context" + "fmt" "os" "github.com/larksuite/cli/extension/credential" @@ -29,28 +30,54 @@ func (p *Provider) ResolveAccount(ctx context.Context) (*credential.Account, err } brand := os.Getenv("LARK_BRAND") if brand == "" { - brand = "lark" + brand = "feishu" } acct := &credential.Account{AppID: appID, AppSecret: appSecret, Brand: brand} + hasUAT := os.Getenv("LARK_USER_ACCESS_TOKEN") != "" + hasTAT := os.Getenv("LARK_TENANT_ACCESS_TOKEN") != "" + + switch defaultAs := os.Getenv("LARKSUITE_CLI_DEFAULT_AS"); defaultAs { + case "", credential.IdentityAuto: + acct.DefaultAs = defaultAs + case credential.IdentityUser, credential.IdentityBot: + acct.DefaultAs = defaultAs + default: + return nil, &credential.BlockError{ + Provider: "env", + Reason: fmt.Sprintf("invalid LARKSUITE_CLI_DEFAULT_AS %q (want user, bot, or auto)", defaultAs), + } + } // Explicit strict mode policy takes priority - switch os.Getenv("LARKSUITE_CLI_STRICT_MODE") { + switch strictMode := os.Getenv("LARKSUITE_CLI_STRICT_MODE"); strictMode { case "bot": acct.SupportedIdentities = credential.SupportsBot case "user": acct.SupportedIdentities = credential.SupportsUser case "off": acct.SupportedIdentities = credential.SupportsAll - default: + case "": // Infer from available tokens - hasUAT := os.Getenv("LARK_USER_ACCESS_TOKEN") != "" - hasTAT := os.Getenv("LARK_TENANT_ACCESS_TOKEN") != "" if hasUAT { acct.SupportedIdentities |= credential.SupportsUser } if hasTAT { acct.SupportedIdentities |= credential.SupportsBot } + default: + return nil, &credential.BlockError{ + Provider: "env", + Reason: fmt.Sprintf("invalid LARKSUITE_CLI_STRICT_MODE %q (want bot, user, or off)", strictMode), + } + } + + if acct.DefaultAs == "" { + switch { + case hasUAT: + acct.DefaultAs = credential.IdentityUser + case hasTAT: + acct.DefaultAs = credential.IdentityBot + } } return acct, nil diff --git a/extension/credential/env/env_test.go b/extension/credential/env/env_test.go index 7f9121974..fbe802889 100644 --- a/extension/credential/env/env_test.go +++ b/extension/credential/env/env_test.go @@ -3,6 +3,7 @@ package env import ( "context" "errors" + "strings" "testing" "github.com/larksuite/cli/extension/credential" @@ -57,8 +58,22 @@ func TestResolveAccount_DefaultBrand(t *testing.T) { t.Setenv("LARK_APP_ID", "cli_test") t.Setenv("LARK_APP_SECRET", "secret_test") acct, _ := (&Provider{}).ResolveAccount(context.Background()) - if acct.Brand != "lark" { - t.Errorf("expected 'lark', got %q", acct.Brand) + if acct.Brand != "feishu" { + t.Errorf("expected 'feishu', got %q", acct.Brand) + } +} + +func TestResolveAccount_DefaultAsFromEnv(t *testing.T) { + t.Setenv("LARK_APP_ID", "cli_test") + t.Setenv("LARK_APP_SECRET", "secret_test") + t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "user") + + acct, err := (&Provider{}).ResolveAccount(context.Background()) + if err != nil { + t.Fatal(err) + } + if acct.DefaultAs != "user" { + t.Errorf("expected default-as user, got %q", acct.DefaultAs) } } @@ -141,6 +156,9 @@ func TestResolveAccount_InferFromUATOnly(t *testing.T) { if !acct.SupportedIdentities.UserOnly() { t.Errorf("expected user-only from UAT inference, got %d", acct.SupportedIdentities) } + if acct.DefaultAs != "user" { + t.Errorf("expected default-as user from UAT inference, got %q", acct.DefaultAs) + } } func TestResolveAccount_InferFromTATOnly(t *testing.T) { @@ -154,6 +172,9 @@ func TestResolveAccount_InferFromTATOnly(t *testing.T) { if !acct.SupportedIdentities.BotOnly() { t.Errorf("expected bot-only from TAT inference, got %d", acct.SupportedIdentities) } + if acct.DefaultAs != "bot" { + t.Errorf("expected default-as bot from TAT inference, got %q", acct.DefaultAs) + } } func TestResolveAccount_InferBothTokens(t *testing.T) { @@ -168,6 +189,9 @@ func TestResolveAccount_InferBothTokens(t *testing.T) { if acct.SupportedIdentities != credential.SupportsAll { t.Errorf("expected SupportsAll, got %d", acct.SupportedIdentities) } + if acct.DefaultAs != "user" { + t.Errorf("expected default-as user when both tokens are present, got %q", acct.DefaultAs) + } } func TestResolveAccount_StrictModeOverridesTokenInference(t *testing.T) { @@ -184,3 +208,39 @@ func TestResolveAccount_StrictModeOverridesTokenInference(t *testing.T) { t.Errorf("strict mode should override token inference, got %d", acct.SupportedIdentities) } } + +func TestResolveAccount_InvalidStrictModeRejected(t *testing.T) { + t.Setenv("LARK_APP_ID", "app") + t.Setenv("LARK_APP_SECRET", "secret") + t.Setenv("LARKSUITE_CLI_STRICT_MODE", "invalid") + + _, err := (&Provider{}).ResolveAccount(context.Background()) + if err == nil { + t.Fatal("expected error for invalid strict mode") + } + var blockErr *credential.BlockError + if !errors.As(err, &blockErr) { + t.Fatalf("expected BlockError, got %T", err) + } + if !strings.Contains(err.Error(), "LARKSUITE_CLI_STRICT_MODE") { + t.Fatalf("error = %v, want mention of LARKSUITE_CLI_STRICT_MODE", err) + } +} + +func TestResolveAccount_InvalidDefaultAsRejected(t *testing.T) { + t.Setenv("LARK_APP_ID", "app") + t.Setenv("LARK_APP_SECRET", "secret") + t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "invalid") + + _, err := (&Provider{}).ResolveAccount(context.Background()) + if err == nil { + t.Fatal("expected error for invalid default-as") + } + var blockErr *credential.BlockError + if !errors.As(err, &blockErr) { + t.Fatalf("expected BlockError, got %T", err) + } + if !strings.Contains(err.Error(), "LARKSUITE_CLI_DEFAULT_AS") { + t.Fatalf("error = %v, want mention of LARKSUITE_CLI_DEFAULT_AS", err) + } +} diff --git a/internal/client/client.go b/internal/client/client.go index a26d9e16e..816b637b1 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -42,6 +43,21 @@ type APIClient struct { Credential *credential.CredentialProvider } +func (c *APIClient) resolveAccessToken(ctx context.Context, as core.Identity) (string, error) { + result, err := c.Credential.ResolveToken(ctx, credential.NewTokenSpec(as, c.Config.AppID)) + if err != nil { + var unavailableErr *credential.TokenUnavailableError + if errors.As(err, &unavailableErr) { + return "", output.ErrAuth("no access token available for %s", as) + } + return "", err + } + if result.Token == "" { + return "", output.ErrAuth("no access token available for %s", as) + } + return result.Token, nil +} + // buildApiReq converts a RawApiRequest into SDK types and collects // request-specific options (ExtraOpts, URL-based headers). // Auth is handled separately by DoSDKRequest. @@ -78,16 +94,16 @@ func (c *APIClient) buildApiReq(request RawApiRequest) (*larkcore.ApiReq, []lark func (c *APIClient) DoSDKRequest(ctx context.Context, req *larkcore.ApiReq, as core.Identity, extraOpts ...larkcore.RequestOptionFunc) (*larkcore.ApiResp, error) { var opts []larkcore.RequestOptionFunc - result, err := c.Credential.ResolveToken(ctx, credential.NewTokenSpec(as, c.Config.AppID)) + token, err := c.resolveAccessToken(ctx, as) if err != nil { return nil, err } if as.IsBot() { req.SupportedAccessTokenTypes = []larkcore.AccessTokenType{larkcore.AccessTokenTypeTenant} - opts = append(opts, larkcore.WithTenantAccessToken(result.Token)) + opts = append(opts, larkcore.WithTenantAccessToken(token)) } else { req.SupportedAccessTokenTypes = []larkcore.AccessTokenType{larkcore.AccessTokenTypeUser} - opts = append(opts, larkcore.WithUserAccessToken(result.Token)) + opts = append(opts, larkcore.WithUserAccessToken(token)) } opts = append(opts, extraOpts...) @@ -105,7 +121,7 @@ func (c *APIClient) DoStream(ctx context.Context, req *larkcore.ApiReq, as core. cfg := buildConfig(opts) // Resolve auth - result, err := c.Credential.ResolveToken(ctx, credential.NewTokenSpec(as, c.Config.AppID)) + token, err := c.resolveAccessToken(ctx, as) if err != nil { return nil, err } @@ -122,12 +138,13 @@ func (c *APIClient) DoStream(ctx context.Context, req *larkcore.ApiReq, as core. return nil, err } - // Timeout + // Timeout — use context deadline only; httpClient.Timeout would cut off + // healthy streaming responses because it includes body read time. httpClient := *c.HTTP + httpClient.Timeout = 0 cancel := func() {} requestCtx := ctx if cfg.timeout > 0 { - httpClient.Timeout = cfg.timeout if _, hasDeadline := ctx.Deadline(); !hasDeadline { requestCtx, cancel = context.WithTimeout(ctx, cfg.timeout) } @@ -150,7 +167,7 @@ func (c *APIClient) DoStream(ctx context.Context, req *larkcore.ApiReq, as core. if contentType != "" { httpReq.Header.Set("Content-Type", contentType) } - httpReq.Header.Set("Authorization", "Bearer "+result.Token) + httpReq.Header.Set("Authorization", "Bearer "+token) resp, err := httpClient.Do(httpReq) if err != nil { diff --git a/internal/client/client_test.go b/internal/client/client_test.go index e0fad8418..658771cd6 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -7,16 +7,20 @@ import ( "bytes" "context" "encoding/json" + "errors" "io" "net/http" + "net/http/httptest" "strings" "testing" + "time" lark "github.com/larksuite/oapi-sdk-go/v3" larkcore "github.com/larksuite/oapi-sdk-go/v3/core" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/credential" + "github.com/larksuite/cli/internal/output" ) // roundTripFunc is an adapter to use a function as http.RoundTripper. @@ -41,6 +45,12 @@ func (s *staticTokenResolver) ResolveToken(_ context.Context, _ credential.Token return &credential.TokenResult{Token: "test-token"}, nil } +type missingTokenResolver struct{} + +func (s *missingTokenResolver) ResolveToken(_ context.Context, _ credential.TokenSpec) (*credential.TokenResult, error) { + return nil, nil +} + // newTestAPIClient creates an APIClient with a mock HTTP transport. func newTestAPIClient(t *testing.T, rt http.RoundTripper) (*APIClient, *bytes.Buffer) { t.Helper() @@ -337,3 +347,78 @@ func TestPaginateAll_NoStreamSummaryLog(t *testing.T) { t.Fatal("expected non-nil result") } } + +func TestDoStream_IgnoresBaseHTTPClientTimeout(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + time.Sleep(25 * time.Millisecond) + _, _ = io.WriteString(w, "ok") + })) + defer srv.Close() + + ac := &APIClient{ + HTTP: &http.Client{Timeout: 5 * time.Millisecond}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + + resp, err := ac.DoStream(context.Background(), &larkcore.ApiReq{ + HttpMethod: http.MethodGet, + ApiPath: srv.URL, + }, core.AsBot) + if err != nil { + t.Fatalf("DoStream() error = %v", err) + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("ReadAll() error = %v", err) + } + if string(body) != "ok" { + t.Fatalf("response body = %q, want %q", string(body), "ok") + } +} + +func TestDoSDKRequest_MissingTokenReturnsAuthError(t *testing.T) { + ac, _ := newTestAPIClient(t, roundTripFunc(func(req *http.Request) (*http.Response, error) { + t.Fatal("unexpected HTTP request") + return nil, nil + })) + ac.Credential = credential.NewCredentialProvider(nil, nil, &missingTokenResolver{}, nil) + + _, err := ac.DoSDKRequest(context.Background(), &larkcore.ApiReq{ + HttpMethod: http.MethodGet, + ApiPath: "/open-apis/test", + }, core.AsBot) + if err == nil { + t.Fatal("DoSDKRequest() error = nil, want auth error") + } + var exitErr *output.ExitError + if !strings.Contains(err.Error(), "no access token available") || !errors.As(err, &exitErr) || exitErr.Detail == nil || exitErr.Detail.Type != "auth" { + t.Fatalf("DoSDKRequest() error = %v, want auth error", err) + } +} + +func TestDoStream_MissingTokenReturnsAuthError(t *testing.T) { + ac := &APIClient{ + HTTP: &http.Client{}, + Credential: credential.NewCredentialProvider(nil, nil, &missingTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{ + HttpMethod: http.MethodGet, + ApiPath: "https://example.com/open-apis/test", + }, core.AsBot) + if err == nil { + t.Fatal("DoStream() error = nil, want auth error") + } + var exitErr *output.ExitError + if !strings.Contains(err.Error(), "no access token available") || !errors.As(err, &exitErr) || exitErr.Detail == nil || exitErr.Detail.Type != "auth" { + t.Fatalf("DoStream() error = %v, want auth error", err) + } +} diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index 4f56e80d6..b50dc1c27 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -8,7 +8,6 @@ import ( "fmt" "io" "net/http" - "os" "strings" lark "github.com/larksuite/oapi-sdk-go/v3" @@ -73,11 +72,8 @@ func (f *Factory) ResolveAs(cmd *cobra.Command, flagAs core.Identity) core.Ident return result } -// resolveDefaultAs returns the configured default identity: env var > config file. +// resolveDefaultAs returns the configured default identity from the resolved account/config. func (f *Factory) resolveDefaultAs() string { - if v := os.Getenv("LARKSUITE_CLI_DEFAULT_AS"); v != "" { - return v - } if cfg, err := f.Config(); err == nil { return cfg.DefaultAs } @@ -86,9 +82,6 @@ func (f *Factory) resolveDefaultAs() string { // autoDetectIdentity checks the login state and returns user if logged in, bot otherwise. func (f *Factory) autoDetectIdentity() core.Identity { - if os.Getenv("LARK_USER_ACCESS_TOKEN") != "" { - return core.AsUser - } cfg, err := f.Config() if err != nil || cfg.UserOpenId == "" { return core.AsBot diff --git a/internal/cmdutil/factory_default.go b/internal/cmdutil/factory_default.go index 259807151..0ecc62f76 100644 --- a/internal/cmdutil/factory_default.go +++ b/internal/cmdutil/factory_default.go @@ -117,11 +117,8 @@ func cachedLarkClientFunc(f *Factory) func() (*lark.Client, error) { lark.WithHeaders(BaseSecurityHeaders()), } util.WarnIfProxied(os.Stderr) - var sdkTransport = http.DefaultTransport - sdkTransport = &UserAgentTransport{Base: sdkTransport} - sdkTransport = &auth.SecurityPolicyTransport{Base: sdkTransport} opts = append(opts, lark.WithHttpClient(&http.Client{ - Transport: sdkTransport, + Transport: buildSDKTransport(), CheckRedirect: safeRedirectPolicy, })) ep := core.ResolveEndpoints(acct.Brand) @@ -130,6 +127,14 @@ func cachedLarkClientFunc(f *Factory) func() (*lark.Client, error) { }) } +func buildSDKTransport() http.RoundTripper { + var sdkTransport http.RoundTripper = util.NewBaseTransport() + sdkTransport = &RetryTransport{Base: sdkTransport} + sdkTransport = &UserAgentTransport{Base: sdkTransport} + sdkTransport = &auth.SecurityPolicyTransport{Base: sdkTransport} + return sdkTransport +} + type credentialDeps struct { Keychain keychain.KeychainAccess Profile string diff --git a/internal/cmdutil/factory_default_test.go b/internal/cmdutil/factory_default_test.go index f67754b0a..f359b844f 100644 --- a/internal/cmdutil/factory_default_test.go +++ b/internal/cmdutil/factory_default_test.go @@ -7,6 +7,8 @@ import ( "errors" "testing" + _ "github.com/larksuite/cli/extension/credential/env" + internalauth "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/core" ) @@ -98,3 +100,39 @@ func TestNewDefault_InvocationProfileMissingSticksAcrossEarlyStrictMode(t *testi t.Fatalf("Config() error message = %q, want %q", cfgErr.Message, `profile "missing" not found`) } } + +func TestBuildSDKTransport_IncludesRetryTransport(t *testing.T) { + transport := buildSDKTransport() + + sec, ok := transport.(*internalauth.SecurityPolicyTransport) + if !ok { + t.Fatalf("outer transport type = %T, want *auth.SecurityPolicyTransport", transport) + } + ua, ok := sec.Base.(*UserAgentTransport) + if !ok { + t.Fatalf("middle transport type = %T, want *UserAgentTransport", sec.Base) + } + if _, ok := ua.Base.(*RetryTransport); !ok { + t.Fatalf("inner transport type = %T, want *RetryTransport", ua.Base) + } +} + +func TestNewDefault_ResolveAs_UsesDefaultAsFromEnvAccount(t *testing.T) { + t.Setenv("LARK_APP_ID", "env-app") + t.Setenv("LARK_APP_SECRET", "env-secret") + t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "user") + t.Setenv("LARK_USER_ACCESS_TOKEN", "") + t.Setenv("LARK_TENANT_ACCESS_TOKEN", "") + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + + f := NewDefault(InvocationContext{}) + cmd := newCmdWithAsFlag("auto", false) + + got := f.ResolveAs(cmd, "auto") + if got != core.AsUser { + t.Fatalf("ResolveAs() = %q, want %q", got, core.AsUser) + } + if f.IdentityAutoDetected { + t.Fatal("IdentityAutoDetected = true, want false") + } +} diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go index 03fd83b6e..a786c211c 100644 --- a/internal/cmdutil/factory_test.go +++ b/internal/cmdutil/factory_test.go @@ -85,15 +85,18 @@ func TestResolveAs_DefaultAs_FromConfig(t *testing.T) { } } -func TestResolveAs_DefaultAs_FromEnv(t *testing.T) { +func TestResolveAs_DefaultAs_EnvDoesNotBypassConfigSource(t *testing.T) { t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "user") f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s"}) cmd := newCmdWithAsFlag("auto", false) got := f.ResolveAs(cmd, "auto") - if got != core.AsUser { - t.Errorf("want user (from env), got %s", got) + if got != core.AsBot { + t.Errorf("want bot (env default-as should not bypass config source), got %s", got) + } + if !f.IdentityAutoDetected { + t.Error("IdentityAutoDetected should be true when no account default-as is set") } } @@ -192,6 +195,16 @@ func TestAutoDetectIdentity_NoUserOpenId(t *testing.T) { } } +func TestAutoDetectIdentity_EnvTokenDoesNotBypassConfigSource(t *testing.T) { + t.Setenv("LARK_USER_ACCESS_TOKEN", "env-uat") + + f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s"}) + got := f.autoDetectIdentity() + if got != core.AsBot { + t.Errorf("want bot (env token should not bypass config source), got %s", got) + } +} + func TestAutoDetectIdentity_ConfigError(t *testing.T) { f := &Factory{ Config: func() (*core.CliConfig, error) { diff --git a/internal/credential/credential_provider.go b/internal/credential/credential_provider.go index 895a9b7d6..23f23de82 100644 --- a/internal/credential/credential_provider.go +++ b/internal/credential/credential_provider.go @@ -24,6 +24,54 @@ type DefaultTokenResolver interface { ResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, error) } +type tokenSource interface { + Name() string + TryResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, bool, error) +} + +type extensionTokenSource struct { + provider extcred.Provider +} + +func (s extensionTokenSource) Name() string { return s.provider.Name() } + +func (s extensionTokenSource) TryResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, bool, error) { + tok, err := s.provider.ResolveToken(ctx, extcred.TokenSpec{ + Type: extcred.TokenType(req.Type.String()), + AppID: req.AppID, + }) + if err != nil { + return nil, false, err + } + if tok == nil { + return nil, false, nil + } + if tok.Value == "" { + return nil, false, fmt.Errorf("credential source %q returned an empty token for %s", s.Name(), req.Type) + } + return &TokenResult{Token: tok.Value, Scopes: tok.Scopes}, true, nil +} + +type defaultTokenSource struct { + resolver DefaultTokenResolver +} + +func (s defaultTokenSource) Name() string { return "default" } + +func (s defaultTokenSource) TryResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, bool, error) { + if s.resolver == nil { + return nil, false, nil + } + result, err := s.resolver.ResolveToken(ctx, req) + if err != nil { + return nil, false, err + } + if result == nil || result.Token == "" { + return nil, false, nil + } + return result, true, nil +} + // CredentialProvider is the unified entry point for all credential resolution. type CredentialProvider struct { providers []extcred.Provider @@ -31,9 +79,10 @@ type CredentialProvider struct { defaultToken DefaultTokenResolver httpClient func() (*http.Client, error) - accountOnce sync.Once - account *Account - accountErr error + accountOnce sync.Once + account *Account + accountErr error + selectedSource tokenSource } // NewCredentialProvider creates a CredentialProvider. @@ -65,18 +114,25 @@ func (p *CredentialProvider) doResolveAccount(ctx context.Context) (*Account, er } if acct != nil { internal := convertAccount(acct) - if err := p.enrichUserInfo(ctx, internal); err != nil { + source := extensionTokenSource{provider: prov} + if err := p.enrichUserInfo(ctx, internal, source); err != nil { // enrichUserInfo failure is non-fatal: SupportedIdentities // (used for strict mode) is already set by the provider. // Clear unverified user identity for safety. internal.UserOpenId = "" internal.UserName = "" } + p.selectedSource = source return internal, nil } } if p.defaultAcct != nil { - return p.defaultAcct.ResolveAccount(ctx) + acct, err := p.defaultAcct.ResolveAccount(ctx) + if err != nil { + return nil, err + } + p.selectedSource = defaultTokenSource{resolver: p.defaultToken} + return acct, nil } return nil, fmt.Errorf("no credential provider returned an account; run 'lark-cli config' to set up") } @@ -84,56 +140,91 @@ func (p *CredentialProvider) doResolveAccount(ctx context.Context) (*Account, er // enrichUserInfo resolves user identity when extension provides a UAT. // If UAT is available, user_info API call is mandatory (security: verify token validity). // If no UAT from extension, falls back to provider-supplied OpenID. -func (p *CredentialProvider) enrichUserInfo(ctx context.Context, acct *Account) error { - if p.httpClient == nil { +func (p *CredentialProvider) enrichUserInfo(ctx context.Context, acct *Account, source tokenSource) error { + if p.httpClient == nil || source == nil { return nil } - for _, prov := range p.providers { - tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{Type: extcred.TokenTypeUAT}) - if err != nil { - var blockErr *extcred.BlockError - if errors.As(err, &blockErr) { - return nil // provider explicitly blocks UAT; skip enrichment - } - continue + tok, found, err := source.TryResolveToken(ctx, TokenSpec{Type: TokenTypeUAT, AppID: acct.AppID}) + if err != nil { + var blockErr *extcred.BlockError + if errors.As(err, &blockErr) { + return nil // provider explicitly blocks UAT; skip enrichment } - if tok == nil { - continue - } - // Have UAT — must verify and resolve identity - hc, err := p.httpClient() - if err != nil { - return fmt.Errorf("failed to get HTTP client for user_info: %w", err) - } - info, err := fetchUserInfo(ctx, hc, acct.Brand, tok.Value) - if err != nil { - return fmt.Errorf("failed to verify user identity: %w", err) - } - acct.UserOpenId = info.OpenID - acct.UserName = info.Name return nil } + if !found { + return nil + } + // Have UAT — must verify and resolve identity + hc, err := p.httpClient() + if err != nil { + return fmt.Errorf("failed to get HTTP client for user_info: %w", err) + } + info, err := fetchUserInfo(ctx, hc, acct.Brand, tok.Token) + if err != nil { + return fmt.Errorf("failed to verify user identity: %w", err) + } + acct.UserOpenId = info.OpenID + acct.UserName = info.Name return nil } +func (p *CredentialProvider) selectedTokenSource(ctx context.Context) (tokenSource, error) { + if p.selectedSource != nil { + return p.selectedSource, nil + } + if p.defaultAcct == nil { + return nil, nil + } + if _, err := p.ResolveAccount(ctx); err != nil { + return nil, err + } + if p.selectedSource == nil { + return nil, fmt.Errorf("credential provider resolved an account without selecting a token source") + } + return p.selectedSource, nil +} + +func resolveTokenFromSource(ctx context.Context, source tokenSource, req TokenSpec) (*TokenResult, error) { + result, found, err := source.TryResolveToken(ctx, req) + if err != nil { + return nil, err + } + if !found { + return nil, &TokenUnavailableError{Source: source.Name(), Type: req.Type} + } + return result, nil +} + // ResolveToken resolves an access token. func (p *CredentialProvider) ResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, error) { + source, err := p.selectedTokenSource(ctx) + if err != nil { + return nil, err + } + if source != nil { + return resolveTokenFromSource(ctx, source, req) + } + for _, prov := range p.providers { - tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{ - Type: extcred.TokenType(req.Type.String()), - AppID: req.AppID, - }) + source := extensionTokenSource{provider: prov} + result, found, err := source.TryResolveToken(ctx, req) if err != nil { return nil, err } - if tok != nil { - return &TokenResult{Token: tok.Value, Scopes: tok.Scopes}, nil + if found { + return result, nil } } - if p.defaultToken != nil { - return p.defaultToken.ResolveToken(ctx, req) + source = defaultTokenSource{resolver: p.defaultToken} + result, found, err := source.TryResolveToken(ctx, req) + if err != nil { + return nil, err } - return nil, fmt.Errorf("no credential provider returned a token for %s", req.Type) + if found { + return result, nil + } + return nil, &TokenUnavailableError{Type: req.Type} } func convertAccount(ext *extcred.Account) *Account { diff --git a/internal/credential/credential_provider_test.go b/internal/credential/credential_provider_test.go index 4ccc7c501..9fddae0de 100644 --- a/internal/credential/credential_provider_test.go +++ b/internal/credential/credential_provider_test.go @@ -3,9 +3,11 @@ package credential import ( "context" "errors" + "net/http" "testing" extcred "github.com/larksuite/cli/extension/credential" + "github.com/larksuite/cli/internal/core" ) type mockExtProvider struct { @@ -101,7 +103,11 @@ func TestCredentialProvider_AccountCached(t *testing.T) { func TestCredentialProvider_TokenFromExtension(t *testing.T) { cp := NewCredentialProvider( - []extcred.Provider{&mockExtProvider{name: "env", token: &extcred.Token{Value: "ext_tok", Source: "env"}}}, + []extcred.Provider{&mockExtProvider{ + name: "env", + account: &extcred.Account{AppID: "ext_app", Brand: "feishu"}, + token: &extcred.Token{Value: "ext_tok", Source: "env"}, + }}, &mockDefaultAcct{}, &mockDefaultToken{result: &TokenResult{Token: "default_tok"}}, nil, ) result, err := cp.ResolveToken(context.Background(), TokenSpec{Type: TokenTypeUAT}) @@ -126,3 +132,107 @@ func TestCredentialProvider_TokenFallsToDefault(t *testing.T) { t.Errorf("expected default_tok, got %s", result.Token) } } + +func TestCredentialProvider_TokenDoesNotMixSourcesAfterDefaultAccountSelection(t *testing.T) { + cp := NewCredentialProvider( + []extcred.Provider{&mockExtProvider{name: "env", token: &extcred.Token{Value: "ext_tok", Source: "env"}}}, + &mockDefaultAcct{account: &Account{AppID: "default_app", Brand: core.BrandFeishu}}, + &mockDefaultToken{result: &TokenResult{Token: "default_tok"}}, + nil, + ) + + if _, err := cp.ResolveAccount(context.Background()); err != nil { + t.Fatalf("ResolveAccount() error = %v", err) + } + + result, err := cp.ResolveToken(context.Background(), TokenSpec{Type: TokenTypeUAT}) + if err != nil { + t.Fatalf("ResolveToken() error = %v", err) + } + if result.Token != "default_tok" { + t.Fatalf("ResolveToken() token = %q, want %q", result.Token, "default_tok") + } +} + +func TestCredentialProvider_SelectedSourceWithoutTokenReturnsUnavailableError(t *testing.T) { + cp := NewCredentialProvider( + []extcred.Provider{&mockExtProvider{ + name: "env", + account: &extcred.Account{AppID: "ext_app", Brand: "feishu"}, + }}, + nil, nil, nil, + ) + + if _, err := cp.ResolveAccount(context.Background()); err != nil { + t.Fatalf("ResolveAccount() error = %v", err) + } + + _, err := cp.ResolveToken(context.Background(), TokenSpec{Type: TokenTypeUAT}) + if err == nil { + t.Fatal("ResolveToken() error = nil, want unavailable error") + } + var unavailableErr *TokenUnavailableError + if !errors.As(err, &unavailableErr) { + t.Fatalf("ResolveToken() error type = %T, want *TokenUnavailableError", err) + } + if unavailableErr.Source != "env" || unavailableErr.Type != TokenTypeUAT { + t.Fatalf("ResolveToken() unavailable error = %+v, want source env and type uat", unavailableErr) + } +} + +func TestCredentialProvider_ResolveTokenPropagatesNonBlockExtensionError(t *testing.T) { + cp := NewCredentialProvider( + []extcred.Provider{&mockExtProvider{name: "env", err: errors.New("provider exploded")}}, + nil, + &mockDefaultToken{result: &TokenResult{Token: "default_tok"}}, + nil, + ) + + _, err := cp.ResolveToken(context.Background(), TokenSpec{Type: TokenTypeUAT}) + if err == nil || err.Error() != "provider exploded" { + t.Fatalf("ResolveToken() error = %v, want provider exploded", err) + } +} + +func TestCredentialProvider_ResolveAccountDoesNotEnrichWithTokenFromDifferentProvider(t *testing.T) { + httpClientCalls := 0 + cp := NewCredentialProvider( + []extcred.Provider{&mockExtProvider{name: "env", token: &extcred.Token{Value: "ext_tok", Source: "env"}}}, + &mockDefaultAcct{account: &Account{ + AppID: "default_app", + Brand: core.BrandFeishu, + UserOpenId: "ou_default", + UserName: "Default User", + }}, + &mockDefaultToken{}, + func() (*http.Client, error) { + httpClientCalls++ + return nil, errors.New("unexpected enrich call") + }, + ) + + acct, err := cp.ResolveAccount(context.Background()) + if err != nil { + t.Fatalf("ResolveAccount() error = %v", err) + } + if httpClientCalls != 0 { + t.Fatalf("httpClient() called %d times, want 0", httpClientCalls) + } + if acct.UserOpenId != "ou_default" || acct.UserName != "Default User" { + t.Fatalf("resolved user = (%q, %q), want (%q, %q)", acct.UserOpenId, acct.UserName, "ou_default", "Default User") + } +} + +func TestCredentialProvider_ResolveTokenDoesNotBypassFailedDefaultAccountResolution(t *testing.T) { + cp := NewCredentialProvider( + nil, + &mockDefaultAcct{err: errors.New("config unavailable")}, + &mockDefaultToken{result: &TokenResult{Token: "default_tok"}}, + nil, + ) + + _, err := cp.ResolveToken(context.Background(), TokenSpec{Type: TokenTypeUAT}) + if err == nil || err.Error() != "config unavailable" { + t.Fatalf("ResolveToken() error = %v, want config unavailable", err) + } +} diff --git a/internal/credential/types.go b/internal/credential/types.go index ce16c4070..c2d79c12f 100644 --- a/internal/credential/types.go +++ b/internal/credential/types.go @@ -2,6 +2,7 @@ package credential import ( "context" + "fmt" "strings" "github.com/larksuite/cli/internal/core" @@ -51,8 +52,22 @@ type TokenResult struct { Scopes string // optional, space-separated; empty = skip scope pre-check } +// TokenUnavailableError reports that no usable token was available. +type TokenUnavailableError struct { + Source string + Type TokenType +} + +func (e *TokenUnavailableError) Error() string { + if e.Source != "" { + return fmt.Sprintf("no %s available from credential source %q", e.Type, e.Source) + } + return fmt.Sprintf("no credential provider returned a token for %s", e.Type) +} + // TokenProvider resolves a runtime access token. -// Returns nil, nil to indicate "I don't handle this, try next provider". +// Top-level resolvers should return a non-nil token or an error. +// Chain participants may use nil, nil internally to indicate "try next source". type TokenProvider interface { ResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, error) } diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index a975f523e..2c7b9bdbd 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -347,12 +347,18 @@ func (ctx *RuntimeContext) OutFormat(data interface{}, meta *output.Meta, pretty // checkScopePrereqs performs a fast local check: does the token // contain all scopes declared by the shortcut? Returns the missing ones. // If scope data is unavailable, returns nil (let the API call handle it). -func checkScopePrereqs(f *cmdutil.Factory, ctx context.Context, appID string, identity core.Identity, required []string) []string { +func checkScopePrereqs(f *cmdutil.Factory, ctx context.Context, appID string, identity core.Identity, required []string) ([]string, error) { result, err := f.Credential.ResolveToken(ctx, credential.NewTokenSpec(identity, appID)) - if err != nil || result == nil || result.Scopes == "" { - return nil + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, err + } + return nil, nil } - return auth.MissingScopes(result.Scopes, required) + if result == nil || result.Scopes == "" { + return nil, nil + } + return auth.MissingScopes(result.Scopes, required), nil } // enhancePermissionError enriches a permission / auth error with the @@ -491,7 +497,10 @@ func checkShortcutScopes(f *cmdutil.Factory, ctx context.Context, as core.Identi if len(scopes) == 0 { return nil } - missing := checkScopePrereqs(f, ctx, config.AppID, as, scopes) + missing, err := checkScopePrereqs(f, ctx, config.AppID, as, scopes) + if err != nil { + return err + } if len(missing) == 0 { return nil } diff --git a/shortcuts/common/runner_scope_test.go b/shortcuts/common/runner_scope_test.go index e7a4efbdb..9d8620b0b 100644 --- a/shortcuts/common/runner_scope_test.go +++ b/shortcuts/common/runner_scope_test.go @@ -4,14 +4,27 @@ package common import ( + "context" "errors" "fmt" "strings" "testing" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/credential" "github.com/larksuite/cli/internal/output" ) +type scopeCheckTokenResolver struct { + result *credential.TokenResult + err error +} + +func (r *scopeCheckTokenResolver) ResolveToken(ctx context.Context, req credential.TokenSpec) (*credential.TokenResult, error) { + return r.result, r.err +} + func TestEnhancePermissionError_MissingScopeType(t *testing.T) { scopes := []string{"calendar:calendar:read"} err := &output.ExitError{ @@ -164,3 +177,25 @@ func TestEnhancePermissionError(t *testing.T) { }) } } + +func TestCheckShortcutScopes_PropagatesContextCancellation(t *testing.T) { + f := &cmdutil.Factory{ + Credential: credential.NewCredentialProvider(nil, nil, &scopeCheckTokenResolver{err: context.Canceled}, nil), + } + + err := checkShortcutScopes(f, context.Background(), core.AsUser, &core.CliConfig{AppID: "app-1"}, []string{"im:message:read"}) + if !errors.Is(err, context.Canceled) { + t.Fatalf("checkShortcutScopes() error = %v, want context.Canceled", err) + } +} + +func TestCheckShortcutScopes_IgnoresNonContextTokenErrors(t *testing.T) { + f := &cmdutil.Factory{ + Credential: credential.NewCredentialProvider(nil, nil, &scopeCheckTokenResolver{err: errors.New("token cache unavailable")}, nil), + } + + err := checkShortcutScopes(f, context.Background(), core.AsUser, &core.CliConfig{AppID: "app-1"}, []string{"im:message:read"}) + if err != nil { + t.Fatalf("checkShortcutScopes() error = %v, want nil", err) + } +}