From a518a03a1ee290a5bc8f44e08c49a04af01bc9c1 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Mon, 17 Aug 2026 07:53:59 +0200 Subject: [PATCH] fix(configure): preserve existing config instead of overwriting it runConfigure never read the existing config: it built a fresh Config with a hardcoded default_provider and an empty Favorites map and saved that, so every re-run of `grant configure` silently destroyed the user's favorites, default_provider and cache_ttl. It now loads the existing config and merges onto a successful load, overwriting only the fields it owns. The rebuild-from-defaults path is kept for the case config.Load fails, which is what keeps configure reachable when the on-disk config is unloadable (no-lockout), and it now warns on stderr instead of discarding the file silently. --- CHANGELOG.md | 1 + CLAUDE.md | 3 +- cmd/configure.go | 33 +++++++--- cmd/configure_test.go | 134 +++++++++++++++++++++++++++++++++++----- docs/mutation-ledger.md | 2 +- 5 files changed, 146 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 704ba07..998a66c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ All notable changes to this project will be documented in this file. ### Fixed +- `grant configure` no longer discards your saved favorites, `default_provider` and `cache_ttl` when re-run - `grant favorites add` now fails immediately without a terminal instead of authenticating first - `grant favorites add`'s non-interactive error now mentions the required favorite name, not only the flags - Interactive selectors now elevate the row you picked, not another target or Entra ID group that happens to render the same way diff --git a/CLAUDE.md b/CLAUDE.md index 006d7aa..cd75563 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -176,7 +176,8 @@ Custom `SCAAccessService` follows SDK conventions: ## Config - App config: `~/.grant/config.yaml` - SDK profile: `~/.idsec/profiles/grant` (default; override via `IDSEC_PROFILES_FOLDER`) -- **`runConfigure` rebuilds the config from scratch** (`cmd/configure.go`): it never reads the existing file, it constructs a fresh `&config.Config{}` with a hardcoded `default_provider` and an empty `Favorites` map and `Save`s that, so every favorite and the user's `default_provider` are silently destroyed. That is also what keeps `grant configure` reachable when the on-disk config is unloadable (the no-lockout property, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`), so it is not purely a bug — but it means **`grant configure` must never be advertised as the remedy for a bad config value**. The user-facing remedy is to edit the file, whose path every load error names. Known sharp edge, follow-up, deliberately not fixed here +- **`runConfigure` merges onto the existing config** (`cmd/configure.go`): it calls `config.Load` and overwrites only what it owns (`profile`), so favorites, `default_provider` and `cache_ttl` survive a re-run (`TestConfigure_PreservesExistingConfigOnRerun`). `Load` treats an absent file as success returning `DefaultConfig()`, so a first run merges onto defaults (`TestConfigure_FirstRunMergesOntoDefaults`) +- **The rebuild-from-defaults fallback is the no-lockout property** and only fires when `Load` *fails*: configure must stay usable when the on-disk config is unloadable, so it warns on stderr and writes a fresh `DefaultConfig()` over the top (`TestConfigure_UnloadableConfigIsRebuiltFromDefaults`). Do not make configure fail on an unloadable config — that locks the user out of the one command that can repair it. On that path the old file cannot be read, so its favorites are genuinely lost; therefore **`grant configure` must still never be advertised as the remedy for a bad config value**. The user-facing remedy is to edit the file, whose path every load error names - Always resolve the profile directory with `profiles.GetProfilesFolder()` (SDK) — never hand-roll it. The SDK reads `os.Getenv("HOME")`, not `os.UserHomeDir()`; on Windows `HOME` is frequently unset, so it resolves to a **relative** `.idsec/profiles` under the process CWD. Any code that prints or computes the profile path must agree with the loader, so reproduce the SDK's behavior rather than "correcting" it ## Keyring diff --git a/cmd/configure.go b/cmd/configure.go index ad2b639..bd96226 100644 --- a/cmd/configure.go +++ b/cmd/configure.go @@ -114,19 +114,34 @@ func runConfigure(cmd *cobra.Command, saver profileSaver, tenantURL, username st profileDir := profiles.GetProfilesFolder() profilePath := filepath.Join(profileDir, "grant") - // Create app config - cfg := &config.Config{ - Profile: "grant", - DefaultProvider: "azure", - Favorites: make(map[string]config.Favorite), - } - - // Save app config - log.Info("Saving config...") + // Merge onto the existing app config so a re-run keeps everything the user + // was not just prompted for (favorites, default_provider, cache_ttl). cfgPath, err := config.ConfigPath() if err != nil { return err } + cfg, err := config.Load(cfgPath) + if err != nil { + // The file exists but cannot be used. Rebuilding from defaults is what + // keeps configure reachable with a broken config (the no-lockout + // property) — but it drops the rest of the file, so say so rather than + // doing it silently. configure is still not the advertised remedy for a + // bad value; editing the file named in the load error is. + fmt.Fprintf(cmd.ErrOrStderr(), + "Warning: existing config %s could not be read (%v); rebuilding it from defaults, any favorites in it are lost\n", + cfgPath, err) + cfg = config.DefaultConfig() + } + cfg.Profile = "grant" + if cfg.Favorites == nil { + cfg.Favorites = make(map[string]config.Favorite) + } + if cfg.DefaultProvider == "" { + cfg.DefaultProvider = config.DefaultConfig().DefaultProvider + } + + // Save app config + log.Info("Saving config...") if err := config.Save(cfg, cfgPath); err != nil { return fmt.Errorf("failed to save config: %w", err) } diff --git a/cmd/configure_test.go b/cmd/configure_test.go index 2695b81..b7a1837 100644 --- a/cmd/configure_test.go +++ b/cmd/configure_test.go @@ -515,21 +515,112 @@ func TestConfigureLongHelpHasNoLegacyPath(t *testing.T) { } } -// TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites pins what -// `grant configure` actually does to a config it cannot load: it never reads the -// old file, it builds a fresh Config from scratch and writes that over the top. +// TestConfigure_PreservesExistingConfigOnRerun pins that a re-run of +// `grant configure` over a LOADABLE config keeps everything the user did not +// answer a prompt for. configure prompts for the username and Identity URL +// only; favorites, default_provider and cache_ttl live in the same file and +// must survive untouched. // -// That keeps configure reachable with a broken config — the no-lockout property -// — but it is NOT an endorsed recovery path for a bad cache_ttl, and no error -// text, doc or help string should point a user at it. runConfigure discards -// every favorite and resets default_provider, so using it to fix a -// one-character typo silently destroys unrelated config. The documented remedy -// is to edit the file named in the load error. The favorites assertion below -// pins that loss so it stays visible; it is recorded behavior, not desired -// behavior, and is flagged as a follow-up in CLAUDE.md. +// Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process. +func TestConfigure_PreservesExistingConfigOnRerun(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + existing := "profile: grant\ndefault_provider: aws\ncache_ttl: 30m\n" + + "favorites:\n prod:\n provider: azure\n target: Prod-EastUS\n role: Contributor\n" + if err := os.WriteFile(cfgPath, []byte(existing), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + t.Setenv("GRANT_CONFIG", cfgPath) + t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(dir, "profiles")) + + cmd := NewConfigureCommandWithDeps(&mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com") + if _, err := executeCommand(cmd); err != nil { + t.Fatalf("configure() error = %v, want nil", err) + } + + cfg, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("Load() after configure = %v", err) + } + if cfg.DefaultProvider != "aws" { + t.Errorf("default_provider = %q after configure, want %q preserved", cfg.DefaultProvider, "aws") + } + if cfg.CacheTTL != "30m" { + t.Errorf("cache_ttl = %q after configure, want %q preserved", cfg.CacheTTL, "30m") + } + fav, ok := cfg.Favorites["prod"] + if !ok { + t.Fatalf("favorites = %v after configure, want the %q favorite preserved", cfg.Favorites, "prod") + } + want := config.Favorite{Provider: "azure", Target: "Prod-EastUS", Role: "Contributor"} + if fav != want { + t.Errorf("favorites[%q] = %+v, want %+v", "prod", fav, want) + } + if cfg.Profile != "grant" { + t.Errorf("profile = %q after configure, want %q", cfg.Profile, "grant") + } +} + +// TestConfigure_FirstRunMergesOntoDefaults pins the absent-config case: +// config.Load reports os.ErrNotExist as a SUCCESS returning DefaultConfig(), so +// a first run merges onto defaults rather than taking the unloadable fallback. // // Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process. -func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) { +func TestConfigure_FirstRunMergesOntoDefaults(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + t.Setenv("GRANT_CONFIG", cfgPath) + t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(dir, "profiles")) + + cmd := NewConfigureCommandWithDeps(&mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com") + out, err := executeCommand(cmd) + if err != nil { + t.Fatalf("configure() error = %v, want nil", err) + } + // The rebuild warning belongs to the unloadable path only; an absent file + // is a normal first run, not a recovery. + if strings.Contains(out, "could not be read") { + t.Errorf("first run warned about an unreadable config\ngot:\n%s", out) + } + + cfg, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("Load() after configure = %v", err) + } + if cfg.Profile != "grant" { + t.Errorf("profile = %q, want %q", cfg.Profile, "grant") + } + if cfg.DefaultProvider != "azure" { + t.Errorf("default_provider = %q, want the default %q", cfg.DefaultProvider, "azure") + } + if cfg.Favorites == nil { + t.Error("favorites = nil, want an initialized map") + } + if len(cfg.Favorites) != 0 { + t.Errorf("favorites = %v, want empty on a first run", cfg.Favorites) + } + if cfg.CacheTTL != "" { + t.Errorf("cache_ttl = %q, want empty on a first run", cfg.CacheTTL) + } +} + +// TestConfigure_UnloadableConfigIsRebuiltFromDefaults pins what `grant +// configure` does to a config it cannot load: it falls back to a fresh Config +// and writes that over the top. +// +// That is what keeps configure reachable with a broken config — the no-lockout +// property — but it is NOT an endorsed recovery path for a bad cache_ttl, and +// no error text, doc or help string should point a user at it. On this path the +// old file cannot be read at all, so favorites and default_provider are lost; +// the assertions below pin that loss so it stays visible, and configure warns +// on stderr rather than doing it silently. The documented remedy for a bad +// value is still to edit the file named in the load error. +// +// (A LOADABLE config is preserved instead — see +// TestConfigure_PreservesExistingConfigOnRerun.) +// +// Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process. +func TestConfigure_UnloadableConfigIsRebuiltFromDefaults(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "config.yaml") broken := "profile: grant\ndefault_provider: aws\ncache_ttl: garbage\n" + @@ -542,9 +633,20 @@ func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) { cmd := NewConfigureCommand() cmd.SetOut(&strings.Builder{}) + stderr := &strings.Builder{} + cmd.SetErr(stderr) err := runConfigure(cmd, &mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com") if err != nil { - t.Fatalf("runConfigure() error = %v, want nil; configure must not read the broken config", err) + t.Fatalf("runConfigure() error = %v, want nil; a broken config must not lock the user out of configure", err) + } + + // The rebuild must be announced, not silent: this is the one path that + // still drops the user's other settings. + if !strings.Contains(stderr.String(), "could not be read") { + t.Errorf("stderr missing the rebuild warning\ngot:\n%s", stderr.String()) + } + if !strings.Contains(stderr.String(), cfgPath) { + t.Errorf("stderr warning does not name the config path %q\ngot:\n%s", cfgPath, stderr.String()) } // The broken value must be gone and the rewritten file must now load. @@ -556,11 +658,11 @@ func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) { t.Errorf("cache_ttl = %q after configure, want it rewritten away", cfg.CacheTTL) } - // The sharp edge: everything else in the file went with it. Pinned so a - // future change to runConfigure has to acknowledge this, and so nobody + // The remaining sharp edge: on this path the old file cannot be read, so + // everything else in it goes with the broken value. Pinned so nobody // mistakes configure for a safe repair tool. if len(cfg.Favorites) != 0 { - t.Errorf("favorites = %v after configure, want them clobbered (pinned pre-existing behavior)", cfg.Favorites) + t.Errorf("favorites = %v after configure, want them gone (unreadable config cannot be merged)", cfg.Favorites) } if cfg.DefaultProvider != "azure" { t.Errorf("default_provider = %q after configure, want the hardcoded %q (the user's \"aws\" is lost)", cfg.DefaultProvider, "azure") diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 3d95afd..a958d0c 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -220,7 +220,7 @@ premise does not hold). | CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | done | | CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`, `TestSessionTimestampRetention_IsTwentyFourHours`, `TestCleanupSessions_IgnoresRetention`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | done | | CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | `TestLoad_UnreadableIsNotTreatedAsMissing`: `Load()` errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | done | -| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately, so it stays reachable with a broken file — but it is **not** the advertised remedy: it rebuilds the config from scratch and drops favorites and `default_provider`, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`. The user-facing remedy is to edit the file named in the load error) | PR6 | done | +| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately, so it stays reachable with a broken file — but it is **not** the advertised remedy: it rebuilds the config from scratch and drops favorites and `default_provider`, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`. The user-facing remedy is to edit the file named in the load error). *Superseded:* `configure` now calls `Load` and merges onto a successful result, falling back to defaults (with a stderr warning) only when the load fails; the no-lockout property and "not the advertised remedy" guidance are unchanged, and the pinning test is now `TestConfigure_UnloadableConfigIsRebuiltFromDefaults` | PR6 | done | | CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | done | | CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | done | | CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | done |