Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions cmd/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ package config
import (
"context"
"errors"
"os"
"path/filepath"
"strings"
"testing"

Expand All @@ -21,6 +23,17 @@ func (n *noopConfigKeychain) Get(service, account string) (string, error) { retu
func (n *noopConfigKeychain) Set(service, account, value string) error { return nil }
func (n *noopConfigKeychain) Remove(service, account string) error { return nil }

type recordingConfigKeychain struct {
removed []string
}

func (r *recordingConfigKeychain) Get(service, account string) (string, error) { return "", nil }
func (r *recordingConfigKeychain) Set(service, account, value string) error { return nil }
func (r *recordingConfigKeychain) Remove(service, account string) error {
r.removed = append(r.removed, service+":"+account)
return nil
}

func TestConfigInitCmd_FlagParsing(t *testing.T) {
f, _, _, _ := cmdutil.TestFactory(t, nil)
f.IOStreams.In = strings.NewReader("secret123\n")
Expand Down Expand Up @@ -221,6 +234,66 @@ func TestConfigRemoveCmd_FlagParsing(t *testing.T) {
}
}

func TestConfigRemoveRun_SaveFailurePreservesExistingConfigAndSecrets(t *testing.T) {
configDir := t.TempDir()
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", configDir)

multi := &core.MultiAppConfig{
Apps: []core.AppConfig{{
AppId: "app-test",
AppSecret: core.SecretInput{
Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:app-test"},
},
Brand: core.BrandFeishu,
Users: []core.AppUser{{UserOpenId: "ou_1", UserName: "Tester"}},
}},
}
if err := core.SaveMultiAppConfig(multi); err != nil {
t.Fatalf("SaveMultiAppConfig() error = %v", err)
}

kc := &recordingConfigKeychain{}
f, _, _, _ := cmdutil.TestFactory(t, nil)
f.Keychain = kc

// Make subsequent config saves fail while keeping the existing config readable.
if err := os.Chmod(configDir, 0500); err != nil {
t.Fatalf("Chmod(%s) error = %v", configDir, err)
}
defer os.Chmod(configDir, 0700)

err := configRemoveRun(&ConfigRemoveOptions{Factory: f})
if err == nil {
t.Fatal("expected save failure")
}
if !strings.Contains(err.Error(), "failed to save config") {
t.Fatalf("error = %v, want failed to save config", err)
}
if len(kc.removed) != 0 {
t.Fatalf("expected no keychain cleanup before successful save, got removals: %v", kc.removed)
}

// Restore permissions and confirm the original config is still intact.
if err := os.Chmod(configDir, 0700); err != nil {
t.Fatalf("restore Chmod(%s) error = %v", configDir, err)
}
saved, err := core.LoadMultiAppConfig()
if err != nil {
t.Fatalf("LoadMultiAppConfig() error = %v", err)
}
if saved == nil || len(saved.Apps) != 1 || saved.Apps[0].AppId != "app-test" {
t.Fatalf("saved config = %#v, want original single app preserved", saved)
}
if got := saved.Apps[0].AppSecret.Ref; got == nil || got.ID != "appsecret:app-test" {
t.Fatalf("saved app secret ref = %#v, want preserved keychain ref", got)
}

configPath := filepath.Join(configDir, "config.json")
if _, err := os.Stat(configPath); err != nil {
t.Fatalf("expected existing config file to remain, stat error = %v", err)
}
}

func TestSaveAsProfile_RejectsProfileNameCollisionWithExistingAppID(t *testing.T) {
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())

Expand Down
14 changes: 8 additions & 6 deletions cmd/config/remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,21 @@ func configRemoveRun(opts *ConfigRemoveOptions) error {
return output.ErrValidation("not configured yet")
}

// Clean up keychain entries for all apps
// 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 := core.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 is cleared.
for _, app := range config.Apps {
core.RemoveSecretStore(app.AppSecret, f.Keychain)
for _, user := range app.Users {
auth.RemoveStoredToken(app.AppId, user.UserOpenId)
}
Comment on lines 57 to 59
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 auth.RemoveStoredToken return value silently discarded

The error from auth.RemoveStoredToken is silently dropped. While RemoveSecretStore is explicitly documented as best-effort, RemoveStoredToken returns an error and callers elsewhere in the codebase may expect failures to be observable. Consider explicitly discarding like the rest of the cleanup pattern:

Suggested change
for _, user := range app.Users {
auth.RemoveStoredToken(app.AppId, user.UserOpenId)
}
_ = auth.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 {
Expand Down
Loading