From 471524d93dc0a147d246300dbbe8ed49ccfcbb08 Mon Sep 17 00:00:00 2001 From: Clio <383320853@qq.com> Date: Tue, 31 Mar 2026 02:29:31 +0000 Subject: [PATCH 01/11] fix: harden auth check and config removal Fixes #116 by rejecting empty --scope values in auth check instead of returning a misleading ok response. Fixes #114 by saving an empty config before best-effort secret/token cleanup, and adds focused regression tests for both flows. --- cmd/auth/check.go | 3 +- cmd/auth/check_test.go | 29 ++++++++ cmd/config/remove.go | 27 +++++--- cmd/config/remove_test.go | 137 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+), 12 deletions(-) create mode 100644 cmd/auth/check_test.go create mode 100644 cmd/config/remove_test.go diff --git a/cmd/auth/check.go b/cmd/auth/check.go index 5f0bd0f4..b6f99797 100644 --- a/cmd/auth/check.go +++ b/cmd/auth/check.go @@ -46,8 +46,7 @@ func authCheckRun(opts *CheckOptions) error { required := strings.Fields(opts.Scope) if len(required) == 0 { - output.PrintJson(f.IOStreams.Out, map[string]interface{}{"ok": true, "granted": []string{}, "missing": []string{}}) - return nil + return output.ErrValidation("--scope cannot be empty") } config, err := f.Config() diff --git a/cmd/auth/check_test.go b/cmd/auth/check_test.go new file mode 100644 index 00000000..d0653816 --- /dev/null +++ b/cmd/auth/check_test.go @@ -0,0 +1,29 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package auth + +import ( + "strings" + "testing" + + "github.com/larksuite/cli/internal/cmdutil" +) + +func TestAuthCheckRun_EmptyScopeReturnsValidationError(t *testing.T) { + f, stdout, _, _ := cmdutil.TestFactory(t, nil) + + err := authCheckRun(&CheckOptions{ + Factory: f, + Scope: " ", + }) + if err == nil { + t.Fatal("expected validation error") + } + if !strings.Contains(err.Error(), "--scope cannot be empty") { + t.Fatalf("unexpected error: %v", err) + } + if stdout.Len() != 0 { + t.Fatalf("expected no stdout, got %q", stdout.String()) + } +} diff --git a/cmd/config/remove.go b/cmd/config/remove.go index 0d7835e8..043fbb8f 100644 --- a/cmd/config/remove.go +++ b/cmd/config/remove.go @@ -13,6 +13,13 @@ import ( "github.com/spf13/cobra" ) +var ( + loadMultiAppConfig = core.LoadMultiAppConfig + saveMultiAppConfig = core.SaveMultiAppConfig + removeSecretStore = core.RemoveSecretStore + removeStoredToken = auth.RemoveStoredToken +) + // ConfigRemoveOptions holds all inputs for config remove. type ConfigRemoveOptions struct { Factory *cmdutil.Factory @@ -39,24 +46,24 @@ func NewCmdConfigRemove(f *cmdutil.Factory, runF func(*ConfigRemoveOptions) erro func configRemoveRun(opts *ConfigRemoveOptions) error { f := opts.Factory - config, err := core.LoadMultiAppConfig() + config, err := loadMultiAppConfig() if err != nil || config == nil || len(config.Apps) == 0 { return output.ErrValidation("not configured yet") } - // Clean up keychain entries for all apps + // Save empty config first so a write failure does not destroy the only recoverable state. + empty := &core.MultiAppConfig{Apps: []core.AppConfig{}} + if err := saveMultiAppConfig(empty); err != nil { + return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) + } + + // Clean up keychain entries for all apps after config has been cleared successfully. for _, app := range config.Apps { - core.RemoveSecretStore(app.AppSecret, f.Keychain) + removeSecretStore(app.AppSecret, f.Keychain) for _, user := range app.Users { - auth.RemoveStoredToken(app.AppId, user.UserOpenId) + _ = removeStoredToken(app.AppId, user.UserOpenId) } } - - // Save empty config - empty := &core.MultiAppConfig{Apps: []core.AppConfig{}} - if err := core.SaveMultiAppConfig(empty); err != nil { - return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) - } output.PrintSuccess(f.IOStreams.ErrOut, "Configuration removed") userCount := 0 for _, app := range config.Apps { diff --git a/cmd/config/remove_test.go b/cmd/config/remove_test.go new file mode 100644 index 00000000..3a501a51 --- /dev/null +++ b/cmd/config/remove_test.go @@ -0,0 +1,137 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package config + +import ( + "errors" + "strings" + "testing" + + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/keychain" +) + +func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) { + f, _, _, _ := cmdutil.TestFactory(t, nil) + + originalLoad := loadMultiAppConfig + originalSave := saveMultiAppConfig + originalRemoveSecretStore := removeSecretStore + originalRemoveStoredToken := removeStoredToken + t.Cleanup(func() { + loadMultiAppConfig = originalLoad + saveMultiAppConfig = originalSave + removeSecretStore = originalRemoveSecretStore + removeStoredToken = originalRemoveStoredToken + }) + + loadMultiAppConfig = func() (*core.MultiAppConfig, error) { + return &core.MultiAppConfig{ + Apps: []core.AppConfig{{ + AppId: "cli_app", + AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app"}}, + Users: []core.AppUser{{UserOpenId: "ou_123"}}, + }}, + }, nil + } + + saveErr := errors.New("disk full") + saveMultiAppConfig = func(*core.MultiAppConfig) error { + return saveErr + } + + secretRemoved := false + tokenRemoved := false + removeSecretStore = func(input core.SecretInput, kc keychain.KeychainAccess) { + secretRemoved = true + } + removeStoredToken = func(appID, userOpenID string) error { + tokenRemoved = true + return nil + } + + err := configRemoveRun(&ConfigRemoveOptions{Factory: f}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "failed to save config: disk full") { + t.Fatalf("unexpected error: %v", err) + } + if secretRemoved { + t.Fatal("secret cleanup should not run when saving empty config fails") + } + if tokenRemoved { + t.Fatal("token cleanup should not run when saving empty config fails") + } +} + +func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) { + f, _, stderr, _ := cmdutil.TestFactory(t, nil) + + originalLoad := loadMultiAppConfig + originalSave := saveMultiAppConfig + originalRemoveSecretStore := removeSecretStore + originalRemoveStoredToken := removeStoredToken + t.Cleanup(func() { + loadMultiAppConfig = originalLoad + saveMultiAppConfig = originalSave + removeSecretStore = originalRemoveSecretStore + removeStoredToken = originalRemoveStoredToken + }) + + cfg := &core.MultiAppConfig{ + Apps: []core.AppConfig{ + { + AppId: "cli_app_a", + AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_a"}}, + Users: []core.AppUser{{UserOpenId: "ou_a"}}, + }, + { + AppId: "cli_app_b", + AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_b"}}, + Users: []core.AppUser{{UserOpenId: "ou_b1"}, {UserOpenId: "ou_b2"}}, + }, + }, + } + loadMultiAppConfig = func() (*core.MultiAppConfig, error) { return cfg, nil } + + savedEmpty := false + saveMultiAppConfig = func(next *core.MultiAppConfig) error { + if len(next.Apps) != 0 { + t.Fatalf("expected empty config, got %+v", next.Apps) + } + savedEmpty = true + return nil + } + + var secretRemovals []string + removeSecretStore = func(input core.SecretInput, kc keychain.KeychainAccess) { + if input.Ref != nil { + secretRemovals = append(secretRemovals, input.Ref.ID) + } + } + + var tokenRemovals []string + removeStoredToken = func(appID, userOpenID string) error { + tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) + return nil + } + + if err := configRemoveRun(&ConfigRemoveOptions{Factory: f}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !savedEmpty { + t.Fatal("expected empty config to be saved before cleanup") + } + if len(secretRemovals) != 2 { + t.Fatalf("secret removals = %v, want 2 entries", secretRemovals) + } + if len(tokenRemovals) != 3 { + t.Fatalf("token removals = %v, want 3 entries", tokenRemovals) + } + if got := stderr.String(); got == "" { + t.Fatal("expected success message on stderr") + } +} From 13cd61e0a927e3eba60a323cc885149715ca6dc2 Mon Sep 17 00:00:00 2001 From: Clio <383320853@qq.com> Date: Tue, 31 Mar 2026 16:36:48 +0000 Subject: [PATCH 02/11] refactor: align config remove with factory injection --- cmd/config/remove.go | 32 ++++++++------ cmd/config/remove_test.go | 90 +++++++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 46 deletions(-) diff --git a/cmd/config/remove.go b/cmd/config/remove.go index 043fbb8f..74a50248 100644 --- a/cmd/config/remove.go +++ b/cmd/config/remove.go @@ -9,25 +9,29 @@ import ( "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/keychain" "github.com/larksuite/cli/internal/output" "github.com/spf13/cobra" ) -var ( - loadMultiAppConfig = core.LoadMultiAppConfig - saveMultiAppConfig = core.SaveMultiAppConfig - removeSecretStore = core.RemoveSecretStore - removeStoredToken = auth.RemoveStoredToken -) - // ConfigRemoveOptions holds all inputs for config remove. type ConfigRemoveOptions struct { - Factory *cmdutil.Factory + Factory *cmdutil.Factory + LoadConfig func() (*core.MultiAppConfig, error) + SaveConfig func(*core.MultiAppConfig) error + RemoveSecret func(core.SecretInput, keychain.KeychainAccess) + RemoveStoredToken func(string, string) error } // NewCmdConfigRemove creates the config remove subcommand. func NewCmdConfigRemove(f *cmdutil.Factory, runF func(*ConfigRemoveOptions) error) *cobra.Command { - opts := &ConfigRemoveOptions{Factory: f} + opts := &ConfigRemoveOptions{ + Factory: f, + LoadConfig: core.LoadMultiAppConfig, + SaveConfig: core.SaveMultiAppConfig, + RemoveSecret: core.RemoveSecretStore, + RemoveStoredToken: auth.RemoveStoredToken, + } cmd := &cobra.Command{ Use: "remove", @@ -46,22 +50,24 @@ func NewCmdConfigRemove(f *cmdutil.Factory, runF func(*ConfigRemoveOptions) erro func configRemoveRun(opts *ConfigRemoveOptions) error { f := opts.Factory - config, err := loadMultiAppConfig() + config, err := opts.LoadConfig() if err != nil || config == nil || len(config.Apps) == 0 { return output.ErrValidation("not configured yet") } // Save empty config first so a write failure does not destroy the only recoverable state. empty := &core.MultiAppConfig{Apps: []core.AppConfig{}} - if err := saveMultiAppConfig(empty); err != nil { + if err := opts.SaveConfig(empty); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) } // Clean up keychain entries for all apps after config has been cleared successfully. for _, app := range config.Apps { - removeSecretStore(app.AppSecret, f.Keychain) + opts.RemoveSecret(app.AppSecret, f.Keychain) for _, user := range app.Users { - _ = removeStoredToken(app.AppId, user.UserOpenId) + if err := opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil { + fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove stored token for app %q user %q: %v\n", app.AppId, user.UserOpenId, err) + } } } output.PrintSuccess(f.IOStreams.ErrOut, "Configuration removed") diff --git a/cmd/config/remove_test.go b/cmd/config/remove_test.go index 3a501a51..e0b24ec2 100644 --- a/cmd/config/remove_test.go +++ b/cmd/config/remove_test.go @@ -15,19 +15,7 @@ import ( func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) { f, _, _, _ := cmdutil.TestFactory(t, nil) - - originalLoad := loadMultiAppConfig - originalSave := saveMultiAppConfig - originalRemoveSecretStore := removeSecretStore - originalRemoveStoredToken := removeStoredToken - t.Cleanup(func() { - loadMultiAppConfig = originalLoad - saveMultiAppConfig = originalSave - removeSecretStore = originalRemoveSecretStore - removeStoredToken = originalRemoveStoredToken - }) - - loadMultiAppConfig = func() (*core.MultiAppConfig, error) { + loadConfig := func() (*core.MultiAppConfig, error) { return &core.MultiAppConfig{ Apps: []core.AppConfig{{ AppId: "cli_app", @@ -38,21 +26,27 @@ func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) { } saveErr := errors.New("disk full") - saveMultiAppConfig = func(*core.MultiAppConfig) error { + saveConfig := func(*core.MultiAppConfig) error { return saveErr } secretRemoved := false tokenRemoved := false - removeSecretStore = func(input core.SecretInput, kc keychain.KeychainAccess) { + removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { secretRemoved = true } - removeStoredToken = func(appID, userOpenID string) error { + removeStoredToken := func(appID, userOpenID string) error { tokenRemoved = true return nil } - err := configRemoveRun(&ConfigRemoveOptions{Factory: f}) + err := configRemoveRun(&ConfigRemoveOptions{ + Factory: f, + LoadConfig: loadConfig, + SaveConfig: saveConfig, + RemoveSecret: removeSecret, + RemoveStoredToken: removeStoredToken, + }) if err == nil { t.Fatal("expected error") } @@ -70,17 +64,6 @@ func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) { func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) { f, _, stderr, _ := cmdutil.TestFactory(t, nil) - originalLoad := loadMultiAppConfig - originalSave := saveMultiAppConfig - originalRemoveSecretStore := removeSecretStore - originalRemoveStoredToken := removeStoredToken - t.Cleanup(func() { - loadMultiAppConfig = originalLoad - saveMultiAppConfig = originalSave - removeSecretStore = originalRemoveSecretStore - removeStoredToken = originalRemoveStoredToken - }) - cfg := &core.MultiAppConfig{ Apps: []core.AppConfig{ { @@ -95,10 +78,10 @@ func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) }, }, } - loadMultiAppConfig = func() (*core.MultiAppConfig, error) { return cfg, nil } + loadConfig := func() (*core.MultiAppConfig, error) { return cfg, nil } savedEmpty := false - saveMultiAppConfig = func(next *core.MultiAppConfig) error { + saveConfig := func(next *core.MultiAppConfig) error { if len(next.Apps) != 0 { t.Fatalf("expected empty config, got %+v", next.Apps) } @@ -107,19 +90,25 @@ func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) } var secretRemovals []string - removeSecretStore = func(input core.SecretInput, kc keychain.KeychainAccess) { + removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { if input.Ref != nil { secretRemovals = append(secretRemovals, input.Ref.ID) } } var tokenRemovals []string - removeStoredToken = func(appID, userOpenID string) error { + removeStoredToken := func(appID, userOpenID string) error { tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) return nil } - if err := configRemoveRun(&ConfigRemoveOptions{Factory: f}); err != nil { + if err := configRemoveRun(&ConfigRemoveOptions{ + Factory: f, + LoadConfig: loadConfig, + SaveConfig: saveConfig, + RemoveSecret: removeSecret, + RemoveStoredToken: removeStoredToken, + }); err != nil { t.Fatalf("unexpected error: %v", err) } if !savedEmpty { @@ -135,3 +124,38 @@ func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) t.Fatal("expected success message on stderr") } } + +func TestConfigRemoveRun_WarnsWhenTokenCleanupFails(t *testing.T) { + f, _, stderr, _ := cmdutil.TestFactory(t, nil) + + loadConfig := func() (*core.MultiAppConfig, error) { + return &core.MultiAppConfig{ + Apps: []core.AppConfig{{ + AppId: "cli_app", + AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app"}}, + Users: []core.AppUser{{UserOpenId: "ou_123"}}, + }}, + }, nil + } + + saveConfig := func(*core.MultiAppConfig) error { return nil } + removeSecret := func(core.SecretInput, keychain.KeychainAccess) {} + removeStoredToken := func(appID, userOpenID string) error { + return errors.New("keychain unavailable") + } + + if err := configRemoveRun(&ConfigRemoveOptions{ + Factory: f, + LoadConfig: loadConfig, + SaveConfig: saveConfig, + RemoveSecret: removeSecret, + RemoveStoredToken: removeStoredToken, + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + got := stderr.String() + if !strings.Contains(got, `warning: failed to remove stored token for app "cli_app" user "ou_123": keychain unavailable`) { + t.Fatalf("expected token cleanup warning, got %q", got) + } +} From 5b2e7488597832eb17f6c39a76f75e2d78a09f8a Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 01:23:18 +0800 Subject: [PATCH 03/11] fix(config): refresh auth-check and config-remove follow-up --- cmd/config/remove.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/config/remove.go b/cmd/config/remove.go index 74a50248..3861b441 100644 --- a/cmd/config/remove.go +++ b/cmd/config/remove.go @@ -48,6 +48,12 @@ func NewCmdConfigRemove(f *cmdutil.Factory, runF func(*ConfigRemoveOptions) erro } func configRemoveRun(opts *ConfigRemoveOptions) error { + if opts == nil || opts.Factory == nil || + opts.LoadConfig == nil || opts.SaveConfig == nil || + opts.RemoveSecret == nil || opts.RemoveStoredToken == nil { + return output.Errorf(output.ExitInternal, "internal", "config remove options not initialized") + } + f := opts.Factory config, err := opts.LoadConfig() @@ -55,13 +61,14 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { return output.ErrValidation("not configured yet") } - // Save empty config first so a write failure does not destroy the only recoverable state. + // Save empty config first. If this fails, keep secrets and tokens intact so the + // existing config can still be retried instead of ending up half-removed. empty := &core.MultiAppConfig{Apps: []core.AppConfig{}} if err := opts.SaveConfig(empty); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) } - // Clean up keychain entries for all apps after config has been cleared successfully. + // Clean up keychain entries for all apps after config is cleared. for _, app := range config.Apps { opts.RemoveSecret(app.AppSecret, f.Keychain) for _, user := range app.Users { @@ -70,6 +77,7 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { } } } + output.PrintSuccess(f.IOStreams.ErrOut, "Configuration removed") userCount := 0 for _, app := range config.Apps { From 26d6d1baf13bab731eb6c2453f2370647247f501 Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 01:23:46 +0800 Subject: [PATCH 04/11] fix(config): refresh auth-check and config-remove follow-up From 6f1638c88815ee2c16469737fc35041694c29db6 Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 01:23:46 +0800 Subject: [PATCH 05/11] fix(config): refresh auth-check and config-remove follow-up --- cmd/auth/check_test.go | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/cmd/auth/check_test.go b/cmd/auth/check_test.go index d0653816..6822438d 100644 --- a/cmd/auth/check_test.go +++ b/cmd/auth/check_test.go @@ -11,19 +11,31 @@ import ( ) func TestAuthCheckRun_EmptyScopeReturnsValidationError(t *testing.T) { - f, stdout, _, _ := cmdutil.TestFactory(t, nil) - - err := authCheckRun(&CheckOptions{ - Factory: f, - Scope: " ", - }) - if err == nil { - t.Fatal("expected validation error") - } - if !strings.Contains(err.Error(), "--scope cannot be empty") { - t.Fatalf("unexpected error: %v", err) + tests := []struct { + name string + scope string + }{ + {name: "empty string", scope: ""}, + {name: "whitespace only", scope: " "}, } - if stdout.Len() != 0 { - t.Fatalf("expected no stdout, got %q", stdout.String()) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, stdout, _, _ := cmdutil.TestFactory(t, nil) + + err := authCheckRun(&CheckOptions{ + Factory: f, + Scope: tt.scope, + }) + if err == nil { + t.Fatal("expected validation error") + } + if !strings.Contains(err.Error(), "--scope cannot be empty") { + t.Fatalf("unexpected error: %v", err) + } + if stdout.Len() != 0 { + t.Fatalf("expected no stdout, got %q", stdout.String()) + } + }) } } From 7323a7052337fb99a52ed122d96f3a799c479f59 Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 01:23:47 +0800 Subject: [PATCH 06/11] fix(config): refresh auth-check and config-remove follow-up --- cmd/config/remove_test.go | 96 ++++++++++++--------------------------- 1 file changed, 28 insertions(+), 68 deletions(-) diff --git a/cmd/config/remove_test.go b/cmd/config/remove_test.go index e0b24ec2..8253bf43 100644 --- a/cmd/config/remove_test.go +++ b/cmd/config/remove_test.go @@ -13,55 +13,7 @@ import ( "github.com/larksuite/cli/internal/keychain" ) -func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) { - f, _, _, _ := cmdutil.TestFactory(t, nil) - loadConfig := func() (*core.MultiAppConfig, error) { - return &core.MultiAppConfig{ - Apps: []core.AppConfig{{ - AppId: "cli_app", - AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app"}}, - Users: []core.AppUser{{UserOpenId: "ou_123"}}, - }}, - }, nil - } - - saveErr := errors.New("disk full") - saveConfig := func(*core.MultiAppConfig) error { - return saveErr - } - - secretRemoved := false - tokenRemoved := false - removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { - secretRemoved = true - } - removeStoredToken := func(appID, userOpenID string) error { - tokenRemoved = true - return nil - } - - err := configRemoveRun(&ConfigRemoveOptions{ - Factory: f, - LoadConfig: loadConfig, - SaveConfig: saveConfig, - RemoveSecret: removeSecret, - RemoveStoredToken: removeStoredToken, - }) - if err == nil { - t.Fatal("expected error") - } - if !strings.Contains(err.Error(), "failed to save config: disk full") { - t.Fatalf("unexpected error: %v", err) - } - if secretRemoved { - t.Fatal("secret cleanup should not run when saving empty config fails") - } - if tokenRemoved { - t.Fatal("token cleanup should not run when saving empty config fails") - } -} - -func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) { +func TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup(t *testing.T) { f, _, stderr, _ := cmdutil.TestFactory(t, nil) cfg := &core.MultiAppConfig{ @@ -78,19 +30,19 @@ func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) }, }, } - loadConfig := func() (*core.MultiAppConfig, error) { return cfg, nil } - savedEmpty := false + callOrder := []string{} saveConfig := func(next *core.MultiAppConfig) error { + callOrder = append(callOrder, "save") if len(next.Apps) != 0 { t.Fatalf("expected empty config, got %+v", next.Apps) } - savedEmpty = true return nil } var secretRemovals []string removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { + callOrder = append(callOrder, "secret") if input.Ref != nil { secretRemovals = append(secretRemovals, input.Ref.ID) } @@ -98,21 +50,23 @@ func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) var tokenRemovals []string removeStoredToken := func(appID, userOpenID string) error { + callOrder = append(callOrder, "token") tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) return nil } if err := configRemoveRun(&ConfigRemoveOptions{ Factory: f, - LoadConfig: loadConfig, + LoadConfig: func() (*core.MultiAppConfig, error) { return cfg, nil }, SaveConfig: saveConfig, RemoveSecret: removeSecret, RemoveStoredToken: removeStoredToken, }); err != nil { t.Fatalf("unexpected error: %v", err) } - if !savedEmpty { - t.Fatal("expected empty config to be saved before cleanup") + + if len(callOrder) == 0 || callOrder[0] != "save" { + t.Fatalf("expected save to happen first, order=%v", callOrder) } if len(secretRemovals) != 2 { t.Fatalf("secret removals = %v, want 2 entries", secretRemovals) @@ -120,8 +74,8 @@ func TestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSave(t *testing.T) if len(tokenRemovals) != 3 { t.Fatalf("token removals = %v, want 3 entries", tokenRemovals) } - if got := stderr.String(); got == "" { - t.Fatal("expected success message on stderr") + if got := stderr.String(); !strings.Contains(got, "Configuration removed") { + t.Fatalf("expected success message on stderr, got %q", got) } } @@ -138,18 +92,14 @@ func TestConfigRemoveRun_WarnsWhenTokenCleanupFails(t *testing.T) { }, nil } - saveConfig := func(*core.MultiAppConfig) error { return nil } - removeSecret := func(core.SecretInput, keychain.KeychainAccess) {} - removeStoredToken := func(appID, userOpenID string) error { - return errors.New("keychain unavailable") - } - if err := configRemoveRun(&ConfigRemoveOptions{ - Factory: f, - LoadConfig: loadConfig, - SaveConfig: saveConfig, - RemoveSecret: removeSecret, - RemoveStoredToken: removeStoredToken, + Factory: f, + LoadConfig: loadConfig, + SaveConfig: func(*core.MultiAppConfig) error { return nil }, + RemoveSecret: func(core.SecretInput, keychain.KeychainAccess) {}, + RemoveStoredToken: func(appID, userOpenID string) error { + return errors.New("keychain unavailable") + }, }); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -159,3 +109,13 @@ func TestConfigRemoveRun_WarnsWhenTokenCleanupFails(t *testing.T) { t.Fatalf("expected token cleanup warning, got %q", got) } } + +func TestConfigRemoveRun_RejectsUninitializedOptions(t *testing.T) { + err := configRemoveRun(&ConfigRemoveOptions{}) + if err == nil { + t.Fatal("expected initialization error") + } + if !strings.Contains(err.Error(), "config remove options not initialized") { + t.Fatalf("unexpected error: %v", err) + } +} From 678f70fc5118f50952ee6803fd5885f96e9838fb Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 14:02:02 +0800 Subject: [PATCH 07/11] fix(config): resolve remove.go merge leftovers --- cmd/config/remove.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cmd/config/remove.go b/cmd/config/remove.go index 10d64a49..3861b441 100644 --- a/cmd/config/remove.go +++ b/cmd/config/remove.go @@ -64,11 +64,7 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { // Save empty config first. If this fails, keep secrets and tokens intact so the // existing config can still be retried instead of ending up half-removed. empty := &core.MultiAppConfig{Apps: []core.AppConfig{}} -<<<<<<< fix/auth-check-and-config-remove if err := opts.SaveConfig(empty); err != nil { -======= - if err := core.SaveMultiAppConfig(empty); err != nil { ->>>>>>> main return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) } @@ -76,13 +72,9 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { for _, app := range config.Apps { opts.RemoveSecret(app.AppSecret, f.Keychain) for _, user := range app.Users { -<<<<<<< fix/auth-check-and-config-remove if err := opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil { fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove stored token for app %q user %q: %v\n", app.AppId, user.UserOpenId, err) } -======= - _ = auth.RemoveStoredToken(app.AppId, user.UserOpenId) ->>>>>>> main } } From 658a41db502e39171432f09e3aea018bdca062fe Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 18:19:11 +0800 Subject: [PATCH 08/11] fix(config): avoid logging raw user identifiers on cleanup warnings --- cmd/config/remove.go => undefined | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename cmd/config/remove.go => undefined (95%) diff --git a/cmd/config/remove.go b/undefined similarity index 95% rename from cmd/config/remove.go rename to undefined index 3861b441..e26ed06b 100644 --- a/cmd/config/remove.go +++ b/undefined @@ -73,7 +73,7 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { opts.RemoveSecret(app.AppSecret, f.Keychain) for _, user := range app.Users { if err := opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil { - fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove stored token for app %q user %q: %v\n", app.AppId, user.UserOpenId, err) + fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove a stored token for app %q: %v\n", app.AppId, err) } } } From ba6bdbccdc72ba8339366ecdb83946af787f9c2c Mon Sep 17 00:00:00 2001 From: Clio Date: Sun, 12 Apr 2026 18:20:21 +0800 Subject: [PATCH 09/11] test(config): avoid asserting leaked user identifiers --- cmd/config/remove_test.go => remove_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) rename cmd/config/remove_test.go => remove_test.go (94%) diff --git a/cmd/config/remove_test.go b/remove_test.go similarity index 94% rename from cmd/config/remove_test.go rename to remove_test.go index 8253bf43..d895aa90 100644 --- a/cmd/config/remove_test.go +++ b/remove_test.go @@ -105,9 +105,12 @@ func TestConfigRemoveRun_WarnsWhenTokenCleanupFails(t *testing.T) { } got := stderr.String() - if !strings.Contains(got, `warning: failed to remove stored token for app "cli_app" user "ou_123": keychain unavailable`) { + if !strings.Contains(got, `warning: failed to remove a stored token for app "cli_app": keychain unavailable`) { t.Fatalf("expected token cleanup warning, got %q", got) } + if strings.Contains(got, "ou_123") { + t.Fatalf("warning should not leak the user identifier, got %q", got) + } } func TestConfigRemoveRun_RejectsUninitializedOptions(t *testing.T) { From 0db2f6778cbd6433420a6de1c474ac144b49ab46 Mon Sep 17 00:00:00 2001 From: Clio Date: Mon, 13 Apr 2026 13:50:46 +0800 Subject: [PATCH 10/11] fix(config): restore remove.go to cmd/config --- undefined => cmd/config/remove.go | 35 ++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) rename undefined => cmd/config/remove.go (78%) diff --git a/undefined b/cmd/config/remove.go similarity index 78% rename from undefined rename to cmd/config/remove.go index e26ed06b..3172f1b7 100644 --- a/undefined +++ b/cmd/config/remove.go @@ -4,7 +4,9 @@ package config import ( + "errors" "fmt" + "os" "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/cmdutil" @@ -57,7 +59,13 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { f := opts.Factory config, err := opts.LoadConfig() - if err != nil || config == nil || len(config.Apps) == 0 { + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return output.ErrValidation("not configured yet") + } + return output.Errorf(output.ExitInternal, "internal", "failed to load config: %v", err) + } + if config == nil || len(config.Apps) == 0 { return output.ErrValidation("not configured yet") } @@ -69,22 +77,35 @@ func configRemoveRun(opts *ConfigRemoveOptions) error { } // Clean up keychain entries for all apps after config is cleared. + tokenTargets := 0 + tokenRemoved := 0 + tokenFailures := 0 for _, app := range config.Apps { opts.RemoveSecret(app.AppSecret, f.Keychain) for _, user := range app.Users { + tokenTargets++ if err := opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil { + tokenFailures++ fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove a stored token for app %q: %v\n", app.AppId, err) + continue } + tokenRemoved++ } } output.PrintSuccess(f.IOStreams.ErrOut, "Configuration removed") - userCount := 0 - for _, app := range config.Apps { - userCount += len(app.Users) - } - if userCount > 0 { - fmt.Fprintf(f.IOStreams.ErrOut, "Cleared tokens for %d users\n", userCount) + if tokenTargets > 0 { + if tokenFailures == 0 { + fmt.Fprintf(f.IOStreams.ErrOut, "Cleared tokens for %d users\n", tokenRemoved) + } else { + fmt.Fprintf( + f.IOStreams.ErrOut, + "Token cleanup attempted for %d users: removed %d, failed %d\n", + tokenTargets, + tokenRemoved, + tokenFailures, + ) + } } return nil } From 6aea3015e4c6e146fcc0f14a0fc94cf3533e8fef Mon Sep 17 00:00:00 2001 From: Clio Date: Mon, 13 Apr 2026 13:51:06 +0800 Subject: [PATCH 11/11] test(config): restore remove_test.go to cmd/config --- remove_test.go => cmd/config/remove_test.go | 81 ++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) rename remove_test.go => cmd/config/remove_test.go (55%) diff --git a/remove_test.go b/cmd/config/remove_test.go similarity index 55% rename from remove_test.go rename to cmd/config/remove_test.go index d895aa90..e904a75f 100644 --- a/remove_test.go +++ b/cmd/config/remove_test.go @@ -5,6 +5,7 @@ package config import ( "errors" + "os" "strings" "testing" @@ -74,9 +75,13 @@ func TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup(t *testing.T if len(tokenRemovals) != 3 { t.Fatalf("token removals = %v, want 3 entries", tokenRemovals) } - if got := stderr.String(); !strings.Contains(got, "Configuration removed") { + got := stderr.String() + if !strings.Contains(got, "Configuration removed") { t.Fatalf("expected success message on stderr, got %q", got) } + if !strings.Contains(got, "Cleared tokens for 3 users") { + t.Fatalf("expected exact cleanup summary, got %q", got) + } } func TestConfigRemoveRun_WarnsWhenTokenCleanupFails(t *testing.T) { @@ -111,6 +116,80 @@ func TestConfigRemoveRun_WarnsWhenTokenCleanupFails(t *testing.T) { if strings.Contains(got, "ou_123") { t.Fatalf("warning should not leak the user identifier, got %q", got) } + if strings.Contains(got, "Cleared tokens for 1 users") { + t.Fatalf("cleanup summary should not claim full success, got %q", got) + } + if !strings.Contains(got, "Token cleanup attempted for 1 users: removed 0, failed 1") { + t.Fatalf("expected partial cleanup summary, got %q", got) + } +} + +func TestConfigRemoveRun_DoesNotCleanupWhenSaveFails(t *testing.T) { + f, _, _, _ := cmdutil.TestFactory(t, nil) + + cfg := &core.MultiAppConfig{ + Apps: []core.AppConfig{{ + AppId: "cli_app", + AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app"}}, + Users: []core.AppUser{{UserOpenId: "ou_123"}}, + }}, + } + + secretCalls := 0 + tokenCalls := 0 + err := configRemoveRun(&ConfigRemoveOptions{ + Factory: f, + LoadConfig: func() (*core.MultiAppConfig, error) { return cfg, nil }, + SaveConfig: func(*core.MultiAppConfig) error { return errors.New("disk full") }, + RemoveSecret: func(core.SecretInput, keychain.KeychainAccess) { + secretCalls++ + }, + RemoveStoredToken: func(string, string) error { + tokenCalls++ + return nil + }, + }) + if err == nil { + t.Fatal("expected save failure") + } + if !strings.Contains(err.Error(), "failed to save config: disk full") { + t.Fatalf("unexpected error: %v", err) + } + if secretCalls != 0 { + t.Fatalf("expected no secret cleanup on save failure, got %d calls", secretCalls) + } + if tokenCalls != 0 { + t.Fatalf("expected no token cleanup on save failure, got %d calls", tokenCalls) + } +} + +func TestConfigRemoveRun_DistinguishesLoadErrors(t *testing.T) { + f, _, _, _ := cmdutil.TestFactory(t, nil) + + notConfiguredErr := configRemoveRun(&ConfigRemoveOptions{ + Factory: f, + LoadConfig: func() (*core.MultiAppConfig, error) { return nil, os.ErrNotExist }, + SaveConfig: func(*core.MultiAppConfig) error { return nil }, + RemoveSecret: func(core.SecretInput, keychain.KeychainAccess) {}, + RemoveStoredToken: func(string, string) error { return nil }, + }) + if notConfiguredErr == nil || !strings.Contains(notConfiguredErr.Error(), "not configured yet") { + t.Fatalf("expected not configured error, got %v", notConfiguredErr) + } + + loadErr := configRemoveRun(&ConfigRemoveOptions{ + Factory: f, + LoadConfig: func() (*core.MultiAppConfig, error) { return nil, errors.New("permission denied") }, + SaveConfig: func(*core.MultiAppConfig) error { return nil }, + RemoveSecret: func(core.SecretInput, keychain.KeychainAccess) {}, + RemoveStoredToken: func(string, string) error { return nil }, + }) + if loadErr == nil { + t.Fatal("expected load failure") + } + if !strings.Contains(loadErr.Error(), "failed to load config: permission denied") { + t.Fatalf("unexpected load error: %v", loadErr) + } } func TestConfigRemoveRun_RejectsUninitializedOptions(t *testing.T) {