From bc3aa89daba109137c651f860c4e680a1ba5b7e1 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 22 Sep 2026 06:47:18 -0400 Subject: [PATCH 1/6] fix: scope OAuth state by profile --- README.md | 2 +- docs/architecture.md | 2 +- internal/auth/auth.go | 36 +++- internal/auth/auth_test.go | 132 +++++++++++++ internal/cache/cache.go | 94 +++------- internal/cache/cache_test.go | 147 +++++---------- internal/cmd/config/behavior_test.go | 8 + internal/cmd/config/config.go | 43 +++-- internal/cmd/init/init.go | 216 +++++++++++++++++---- internal/cmd/init/init_test.go | 43 +++++ internal/cmd/init/sibling_test.go | 24 +++ internal/cmd/me/me.go | 15 +- internal/cmd/profiles/profiles_test.go | 7 + internal/config/config.go | 249 +++++++++++++++++++++++-- internal/config/config_test.go | 91 +++++++++ internal/config/identity.go | 64 ++++++- internal/config/relocate.go | 108 ++++++++++- internal/config/relocate_test.go | 52 ++++++ internal/e2e/profile_scoped_test.go | 171 +++++++++++++++++ internal/keychain/keychain.go | 17 ++ internal/keychain/migrate.go | 39 +++- 21 files changed, 1297 insertions(+), 263 deletions(-) create mode 100644 internal/e2e/profile_scoped_test.go diff --git a/README.md b/README.md index 3b39ed5..3f38511 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ grw contacts create --given-name Test --email t@example.com --dry-run grw drive trash --query "name contains 'old'" --dry-run ``` -One desktop OAuth client can be used by both tools, but each tool asks for consent and stores its token under its own identity. Google Workspace administrators should start with [`WORKSPACE_ADMINS.md`](WORKSPACE_ADMINS.md). +Each `--profile` selects one complete account context: its token, OAuth client JSON, granted scopes, and Drive metadata cache. `init --profile work --credentials-file client.json` binds that client to `work`; an unconfigured profile never falls back to another profile's client or cache. The same desktop OAuth client may be reused by both tools for the same profile, but each tool still asks for consent and stores its token under its own identity. Google Workspace administrators should start with [`WORKSPACE_ADMINS.md`](WORKSPACE_ADMINS.md). ## Documentation diff --git a/docs/architecture.md b/docs/architecture.md index 7cf39de..d37634b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -73,7 +73,7 @@ The write `NewCommand()` starts with `internal/cmd/.NewCommand()` and at `cmd/grw/main.go` registers `internal/app/grw.Identity()` before running the `grw` root. That root composes setup/configuration and profile commands with the extended Gmail, Calendar, Contacts, and Drive commands. -Both identities drive the same config, cache, credential-reference, and keyring code. They use different directory names, default credential references, environment-variable prefixes, and keyring namespaces. Their OAuth client JSON may be reused across identities, but tokens and consent remain separate. `gro` requests its non-destructive multi-service scopes; `grw` requests Gmail write scopes, Calendar, Contacts, and Drive read/write scopes, and basic profile access. +Both identities drive the same config, cache, credential-reference, and keyring code. They use different directory names, default credential references, environment-variable prefixes, and keyring namespaces. Each bare profile owns its OAuth client JSON, granted scopes, token, and Drive cache; a client JSON may be reused across identities only for the same profile, while tokens and consent remain separate. `gro` requests its non-destructive multi-service scopes; `grw` requests Gmail write scopes, Calendar, Contacts, and Drive read/write scopes, and basic profile access. ## Structural enforcement diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5d6d16d..da57080 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -57,15 +57,29 @@ func CheckScopesMigration(grantedScopes []string) string { return msg } -// GetOAuthConfig loads the OAuth client config from the deployment-material -// OAuth client JSON referenced by config.yml's oauth_client_path (§1.2 — not -// a secret; lives on disk, never the keyring), with all scopes. +// GetOAuthConfig loads the OAuth client for the effective credential ref. The +// ref is resolved without opening the keyring so all account-derived state +// follows the same selection boundary. func GetOAuthConfig() (*oauth2.Config, error) { + ref, err := keychain.ResolveEffectiveCredentialRef() + if err != nil { + return nil, err + } + return GetOAuthConfigForRef(ref) +} + +// GetOAuthConfigForRef loads the exact profile-owned OAuth client JSON for +// ref. An unconfigured profile is a hard miss; it never falls back to the +// active profile's client or scopes. +func GetOAuthConfigForRef(ref string) (*oauth2.Config, error) { cfg, err := config.LoadConfigForRuntime() if err != nil { return nil, err } - path := config.ExpandPath(cfg.OAuthClientPath) + path := cfg.OAuthClientPathForRef(ref) + if path == "" { + return nil, fmt.Errorf("no OAuth client configured for credential %s (run '%s init --profile %s --credentials-file ' or import a client for this profile)", ref, config.ProductName(), profileName(ref)) + } b, err := os.ReadFile(path) //nolint:gosec // deployment-material path from config if err != nil { return nil, fmt.Errorf("unable to read OAuth client JSON %s (run '%s init'): %w", @@ -106,7 +120,9 @@ func GetHTTPClientForRef(ctx context.Context, ref string) (*http.Client, error) // lifetime). Shared by the active-ref and explicit-ref entry points so the // persist-on-refresh and error-attribution behavior can't diverge. func clientFromStore(ctx context.Context, st *keychain.Store) (*http.Client, error) { - oauthCfg, err := GetOAuthConfig() + ref := st.Ref() + refSource := st.RefSource() + oauthCfg, err := GetOAuthConfigForRef(ref) if err != nil { _ = st.Close() return nil, err @@ -118,8 +134,6 @@ func clientFromStore(ctx context.Context, st *keychain.Store) (*http.Client, err return nil, fmt.Errorf("no OAuth token stored for credential %s (selected via %s) - run '%s init' first: %w", st.Ref(), keychain.DescribeRefSource(st.RefSource()), config.ProductName(), err) } - ref := st.Ref() - refSource := st.RefSource() _ = st.Close() // do not hold the Store for the client's lifetime persist := func(t *oauth2.Token) error { @@ -135,6 +149,14 @@ func clientFromStore(ctx context.Context, st *keychain.Store) (*http.Client, err return oauth2.NewClient(ctx, &attributedTokenSource{base: tokenSource, ref: ref, source: refSource}), nil } +func profileName(ref string) string { + _, profile, ok := strings.Cut(ref, "/") + if !ok { + return ref + } + return profile +} + // attributedTokenSource decorates auth failures from the wrapped source with // the credential ref that failed and where that ref was selected. Without // this, an expired/revoked refresh token surfaces as a bare diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 6cb28ed..a72a9f2 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1,14 +1,146 @@ package auth import ( + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" "testing" + "time" + + "golang.org/x/oauth2" "github.com/open-cli-collective/google-cli/internal/config" + "github.com/open-cli-collective/google-cli/internal/credtest" + "github.com/open-cli-collective/google-cli/internal/keychain" ) +func clientJSON(clientID, tokenURI string) string { + return fmt.Sprintf(`{"installed":{"client_id":%q,"project_id":"p","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":%q,"client_secret":"secret","redirect_uris":["http://localhost"]}}`, clientID, tokenURI) +} + +func TestGetOAuthConfigForRefUsesExactProfileClient(t *testing.T) { + credtest.Setup(t) + dir := credtest.ConfigDir(t) + defaultPath := filepath.Join(dir, "default-client.json") + workPath := filepath.Join(dir, "work-client.json") + if err := os.WriteFile(defaultPath, []byte(clientJSON("default-client", "https://oauth2.googleapis.com/token")), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(workPath, []byte(clientJSON("work-client", "https://oauth2.googleapis.com/token")), 0o600); err != nil { + t.Fatal(err) + } + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": {OAuthClientPath: defaultPath, GrantedScopes: []string{"default-scope"}}, + "work": {OAuthClientPath: workPath, GrantedScopes: []string{"work-scope"}}, + }, + }); err != nil { + t.Fatal(err) + } + got, err := GetOAuthConfigForRef("google-readonly/work") + if err != nil { + t.Fatal(err) + } + if got.ClientID != "work-client" { + t.Fatalf("work client ID = %q, want work-client", got.ClientID) + } + if _, err := GetOAuthConfigForRef("google-readonly/other"); err == nil || !strings.Contains(err.Error(), "no OAuth client configured") { + t.Fatalf("unconfigured ref result = %v, want actionable miss", err) + } +} + +func TestGetHTTPClientForRefUsesMatchingOAuthClientOnRefresh(t *testing.T) { + credtest.Setup(t) + const ref = "google-readonly/work" + seenClientID := make(chan string, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/token": + if err := r.ParseForm(); err != nil { + http.Error(w, "bad form", http.StatusBadRequest) + return + } + clientID := r.Form.Get("client_id") + if clientID == "" { + clientID, _, _ = r.BasicAuth() + } + seenClientID <- clientID + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"access_token":"work-access","token_type":"Bearer","expires_in":3600}`) + case "/api": + if r.Header.Get("Authorization") != "Bearer work-access" { + http.Error(w, "wrong token", http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusNoContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + dir := credtest.ConfigDir(t) + defaultPath := filepath.Join(dir, "default-client.json") + workPath := filepath.Join(dir, "work-client.json") + if err := os.WriteFile(defaultPath, []byte(clientJSON("default-client", server.URL+"/token")), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(workPath, []byte(clientJSON("work-client", server.URL+"/token")), 0o600); err != nil { + t.Fatal(err) + } + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": {OAuthClientPath: defaultPath}, + "work": {OAuthClientPath: workPath}, + }, + }); err != nil { + t.Fatal(err) + } + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatal(err) + } + if err := st.SetToken(&oauth2.Token{AccessToken: "expired", RefreshToken: "refresh", Expiry: time.Now().Add(-time.Hour)}); err != nil { + _ = st.Close() + t.Fatal(err) + } + if err := st.Close(); err != nil { + t.Fatal(err) + } + + httpClient, err := GetHTTPClientForRef(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL+"/api", nil) + if err != nil { + t.Fatal(err) + } + response, err := httpClient.Do(request) + if err != nil { + t.Fatal(err) + } + _ = response.Body.Close() + if response.StatusCode != http.StatusNoContent { + t.Fatalf("API status = %d, want %d", response.StatusCode, http.StatusNoContent) + } + select { + case got := <-seenClientID: + if got != "work-client" { + t.Fatalf("refresh client ID = %q, want work-client", got) + } + case <-time.After(2 * time.Second): + t.Fatal("refresh endpoint was not called") + } +} + // TestDeprecatedWrappers verifies that auth package wrappers delegate to config package func TestDeprecatedWrappers(t *testing.T) { t.Run("GetConfigDir delegates to config package", func(t *testing.T) { diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 16c5151..6e1576d 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -18,20 +18,18 @@ import ( "time" clicache "github.com/open-cli-collective/cli-common/cache" + "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/google-cli/internal/config" + "github.com/open-cli-collective/google-cli/internal/keychain" ) const ( - // DrivesFile is the cache file for shared drives. Retained as the legacy - // pre-B2b file name so migrateLegacyCacheDir can find and relocate it. + // DrivesFile is the cache file for shared drives. DrivesFile = "drives.json" // drivesResource is the cli-common cache resource name. The on-disk file // becomes //drives.json. drivesResource = "drives" - // instanceKey is gro's single-instance discriminator (Locator requires - // one). Per cli-common docs: single-instance CLIs use "default". - instanceKey = "default" // drivesTTL is the §4.4 hard-coded per-resource TTL for shared drives — // same 24-hour default the user-configurable knob previously defaulted to. drivesTTL = "24h" @@ -50,82 +48,34 @@ type Cache struct { } // New creates a new Cache instance rooted at the OS cache dir (B2b via -// cli-common/statedir). It also runs a transparent, best-effort one-time -// relocation of a pre-B2b cache that lived inside the config dir; relocation -// never fails New (the cache is disposable — it simply repopulates). +// cli-common/statedir), isolated by the selected bare credential profile. +// Legacy unscoped cache is intentionally ignored: it is disposable and cannot +// be safely attributed to any profile. func New() (*Cache, error) { + ref, err := keychain.ResolveEffectiveCredentialRef() + if err != nil { + // Package-level tests that construct infrastructure before registering an + // identity have no effective ref; keep their cache hermetic while real + // binaries always register one before command execution. + if config.DefaultCredentialRef == "" { + ref = "test/default" + } else { + return nil, err + } + } + _, profile, err := credstore.ParseRef(ref) + if err != nil { + return nil, fmt.Errorf("invalid credential ref %q: %w", ref, err) + } cacheDir, err := config.GetCacheDir() if err != nil { return nil, err } - migrateLegacyCacheDir(cacheDir) return &Cache{ - loc: clicache.Locator{Root: cacheDir, InstanceKey: instanceKey}, + loc: clicache.Locator{Root: cacheDir, InstanceKey: profile}, }, nil } -// migrateLegacyCacheDir relocates a pre-B2b cache (a "cache" subdir of the -// config dir) into newDir, then removes the legacy dir. Strictly silent and -// best-effort: any failure is abandoned without touching New's result. If the -// warm-cache carry fails the legacy dir is left intact; a stuck legacy dir is -// force-cleaned by `config clear --all`. The carry is a byte copy and does -// NOT parse the envelope; a stale shape arriving in newDir is harmless -// because cli-common/cache's identity/version check + this package's -// parse-error-mapped-to-miss demote it on the next read. -func migrateLegacyCacheDir(newDir string) { - // Two possible legacy locations on macOS/Windows: (a) the pre-B2b - // "cache" subdir under the NEW (statedir-resolved) config dir — what a - // user reached if they already ran post-port code once — and (b) the - // same subdir under the OLD hand-rolled config dir, which is where a - // pure pre-MON-5371 install lived. Both are byte-carried best-effort; - // any failure abandons the migration without touching New's result. On - // Linux the two paths collapse, so the second probe is a stat-only no-op. - probes := []func() (string, error){ - config.LegacyCacheDir, - config.OldHandRolledLegacyCacheDir, - } - tried := map[string]bool{} - for _, p := range probes { - legacy, err := p() - if err != nil || tried[legacy] { - continue - } - tried[legacy] = true - tryCarryLegacyCache(legacy, newDir) - } -} - -func tryCarryLegacyCache(legacy, newDir string) { - if _, err := os.Stat(legacy); err != nil { - return - } - - legacyDrives := filepath.Join(legacy, DrivesFile) - newDrives := filepath.Join(newDir, instanceKey, DrivesFile) - switch _, serr := os.Stat(newDrives); { - case serr == nil: - // New cache already present: nothing to carry, safe to drop legacy. - case os.IsNotExist(serr): - data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // path from legacy config dir - switch { - case rerr == nil: - if mkErr := os.MkdirAll(filepath.Dir(newDrives), 0o700); mkErr != nil { - return - } - if werr := os.WriteFile(newDrives, data, config.TokenPerm); werr != nil { //nolint:gosec // disposable cache - return - } - case os.IsNotExist(rerr): - // No warm file to carry — fine, fall through to cleanup. - default: - return - } - default: - return - } - _ = os.RemoveAll(legacy) -} - // GetDrives returns cached shared drives, or nil if cache is stale, missing, // or corrupt. Corrupt-as-miss preserves the pre-MON-5371 behavior: caches // are disposable, so a JSON parse error self-heals on the next API call. I/O diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index b947c09..dca3d52 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -9,6 +9,7 @@ import ( "github.com/open-cli-collective/cli-common/statedirtest" "github.com/open-cli-collective/google-cli/internal/config" + "github.com/open-cli-collective/google-cli/internal/keychain" "github.com/open-cli-collective/google-cli/internal/testutil" ) @@ -69,6 +70,31 @@ func TestCache_GetSetDrives(t *testing.T) { }) } +func TestCacheIsolatedBySelectedProfile(t *testing.T) { + hermetic(t) + keychain.SetCredentialRefOverride("google-readonly/default", true) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + defaultCache, err := New() + testutil.NoError(t, err) + testutil.NoError(t, defaultCache.SetDrives([]*CachedDrive{{ID: "default", Name: "Default"}})) + + keychain.SetCredentialRefOverride("google-readonly/work", true) + workCache, err := New() + testutil.NoError(t, err) + got, err := workCache.GetDrives() + testutil.NoError(t, err) + testutil.Nil(t, got) + testutil.NoError(t, workCache.SetDrives([]*CachedDrive{{ID: "work", Name: "Work"}})) + + keychain.SetCredentialRefOverride("google-readonly/default", true) + got, err = defaultCache.GetDrives() + testutil.NoError(t, err) + testutil.Len(t, got, 1) + testutil.Equal(t, got[0].ID, "default") + defaultCache.Clear() + workCache.Clear() +} + func TestCache_Expiration(t *testing.T) { hermetic(t) c, err := New() @@ -233,109 +259,22 @@ func TestCache_GetDir(t *testing.T) { testutil.Equal(t, c.GetDir(), want) } -func TestMigrateLegacyCacheDir(t *testing.T) { - setup := func(t *testing.T) (legacy, newDir string) { - t.Helper() - hermetic(t) - legacy, err := config.LegacyCacheDir() - testutil.NoError(t, err) - newDir, err = config.CacheDirPath() - testutil.NoError(t, err) - return legacy, newDir - } - seedLegacy := func(t *testing.T, legacy, payload string) { - t.Helper() - testutil.NoError(t, os.MkdirAll(legacy, 0o700)) - testutil.NoError(t, os.WriteFile(filepath.Join(legacy, DrivesFile), []byte(payload), 0o600)) - } - - t.Run("carries warm cache then removes legacy", func(t *testing.T) { - legacy, newDir := setup(t) - seedLegacy(t, legacy, `{"cached_at":"2026-01-01T00:00:00Z","ttl_hours":24,"drives":[{"id":"d1","name":"Eng"}]}`) - - c, err := New() - testutil.NoError(t, err) - defer c.Clear() - - got, err := os.ReadFile(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) - testutil.NoError(t, err) - testutil.Contains(t, string(got), `"d1"`) - _, statErr := os.Stat(legacy) - testutil.True(t, os.IsNotExist(statErr)) - - // Idempotent: a second New is a no-op and keeps the new cache. - c2, err := New() - testutil.NoError(t, err) - defer c2.Clear() - _, err = os.Stat(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) - testutil.NoError(t, err) - }) - - t.Run("does not overwrite an existing new cache; still removes legacy", func(t *testing.T) { - legacy, _ := setup(t) - c, err := New() - testutil.NoError(t, err) - defer c.Clear() - testutil.NoError(t, c.SetDrives([]*CachedDrive{{ID: "new", Name: "Keep"}})) - seedLegacy(t, legacy, `{"drives":[{"id":"old","name":"Stale"}]}`) - - c2, err := New() - testutil.NoError(t, err) - defer c2.Clear() - - drives, err := c2.GetDrives() - testutil.NoError(t, err) - testutil.Len(t, drives, 1) - testutil.Equal(t, drives[0].ID, "new") - _, statErr := os.Stat(legacy) - testutil.True(t, os.IsNotExist(statErr)) - }) - - t.Run("unreadable legacy drives file: legacy preserved, no partial carry", func(t *testing.T) { - legacy, newDir := setup(t) - // drives.json as a directory => os.ReadFile errors with a - // non-IsNotExist error => carry fails => legacy must NOT be removed - // and nothing must be written into the new cache. - testutil.NoError(t, os.MkdirAll(filepath.Join(legacy, DrivesFile), 0o700)) - - c, err := New() - testutil.NoError(t, err) - defer c.Clear() - - _, statErr := os.Stat(legacy) - testutil.NoError(t, statErr) - _, newErr := os.Stat(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) - testutil.True(t, os.IsNotExist(newErr)) - }) - - t.Run("no legacy is a clean no-op", func(t *testing.T) { - setup(t) - c, err := New() - testutil.NoError(t, err) - defer c.Clear() - }) - - t.Run("old-hand-rolled legacy cache subdir is carried (pre-MON-5371 install)", func(t *testing.T) { - hermetic(t) - oldLegacy, err := config.OldHandRolledLegacyCacheDir() - testutil.NoError(t, err) - newDir, err := config.CacheDirPath() - testutil.NoError(t, err) - // On Linux this is the same path as LegacyCacheDir — dedup handles it. - // On macOS/Windows this is the path a pure pre-MON-5371 install lived - // at and is what the dual-probe needs to find. - testutil.NoError(t, os.MkdirAll(oldLegacy, 0o700)) - testutil.NoError(t, os.WriteFile(filepath.Join(oldLegacy, DrivesFile), - []byte(`{"cached_at":"2026-01-01T00:00:00Z","ttl_hours":24,"drives":[{"id":"hand","name":"R"}]}`), 0o600)) - - c, err := New() - testutil.NoError(t, err) - defer c.Clear() +func TestLegacyCacheIsDisposable(t *testing.T) { + hermetic(t) + legacy, err := config.LegacyCacheDir() + testutil.NoError(t, err) + testutil.NoError(t, os.MkdirAll(legacy, 0o700)) + testutil.NoError(t, os.WriteFile(filepath.Join(legacy, DrivesFile), + []byte(`{"drives":[{"id":"old","name":"Stale"}]}`), 0o600)) - got, err := os.ReadFile(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) - testutil.NoError(t, err) - testutil.Contains(t, string(got), `"hand"`) - _, statErr := os.Stat(oldLegacy) - testutil.True(t, os.IsNotExist(statErr)) - }) + c, err := New() + testutil.NoError(t, err) + defer c.Clear() + got, err := c.GetDrives() + testutil.NoError(t, err) + testutil.Nil(t, got) + // The old unscoped cache is neither guessed into this profile nor removed + // by normal cache construction; config clear --all owns broad cleanup. + _, statErr := os.Stat(filepath.Join(legacy, DrivesFile)) + testutil.NoError(t, statErr) } diff --git a/internal/cmd/config/behavior_test.go b/internal/cmd/config/behavior_test.go index c3638dc..30f9c74 100644 --- a/internal/cmd/config/behavior_test.go +++ b/internal/cmd/config/behavior_test.go @@ -50,6 +50,14 @@ func seedTokenAndClient(t *testing.T) string { if err := os.WriteFile(filepath.Join(dir, appconfig.OAuthClientFile), []byte(clientJSON), 0o644); err != nil { t.Fatal(err) } + if err := appconfig.SaveConfig(&appconfig.Config{ + CredentialRef: appconfig.DefaultCredentialRef, + Profiles: map[string]appconfig.ProfileConfig{ + "default": {OAuthClientPath: filepath.Join(dir, appconfig.OAuthClientFile)}, + }, + }); err != nil { + t.Fatal(err) + } st, err := keychain.OpenNoMigrate() if err != nil { t.Fatal(err) diff --git a/internal/cmd/config/config.go b/internal/cmd/config/config.go index 355042e..96e30e4 100644 --- a/internal/cmd/config/config.go +++ b/internal/cmd/config/config.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/spf13/cobra" @@ -125,17 +126,18 @@ The OAuth client JSON (deployment material) is never removed.`, // showStatus is the §1.6 non-secret view: never the token value, not even a // masked prefix. type showStatus struct { - CredentialRef string `json:"credential_ref"` - CredentialRefSource string `json:"credential_ref_source,omitempty"` - Backend string `json:"backend"` - BackendSource string `json:"backend_source"` - KeyringBackend string `json:"keyring_backend,omitempty"` // selector from config.yml (keyring.backend) - PassphraseSource string `json:"passphrase_source,omitempty"` - OAuthTokenPresent bool `json:"oauth_token_present"` - OAuthClientPath string `json:"oauth_client_path"` - OAuthClientPresent bool `json:"oauth_client_present"` - OAuthClientFingerprint string `json:"oauth_client_fingerprint,omitempty"` - OAuthClientContents string `json:"oauth_client_contents,omitempty"` + CredentialRef string `json:"credential_ref"` + CredentialRefSource string `json:"credential_ref_source,omitempty"` + Backend string `json:"backend"` + BackendSource string `json:"backend_source"` + KeyringBackend string `json:"keyring_backend,omitempty"` // selector from config.yml (keyring.backend) + PassphraseSource string `json:"passphrase_source,omitempty"` + OAuthTokenPresent bool `json:"oauth_token_present"` + OAuthClientPath string `json:"oauth_client_path"` + OAuthClientPresent bool `json:"oauth_client_present"` + OAuthClientFingerprint string `json:"oauth_client_fingerprint,omitempty"` + OAuthClientContents string `json:"oauth_client_contents,omitempty"` + GrantedScopes []string `json:"granted_scopes,omitempty"` } func runShow(jsonOut, verbose bool) error { @@ -158,6 +160,7 @@ func runShow(jsonOut, verbose bool) error { return err } backend, src := st.Backend() + clientPath := cfg.OAuthClientPathForRef(st.Ref()) status := showStatus{ CredentialRef: st.Ref(), CredentialRefSource: string(st.RefSource()), @@ -165,17 +168,20 @@ func runShow(jsonOut, verbose bool) error { BackendSource: string(src), KeyringBackend: cfg.Keyring.Backend, // selector value from config.yml; "" if unset OAuthTokenPresent: hasTok, - OAuthClientPath: config.ShortenPath(cfg.OAuthClientPath), + OAuthClientPath: config.ShortenPath(clientPath), OAuthClientPresent: false, + GrantedScopes: cfg.GrantedScopesForRef(st.Ref()), } if backend == credstore.BackendFile { status.PassphraseSource = keychain.PassphraseSource(st.Service()) } - if data, rerr := os.ReadFile(cfg.OAuthClientPath); rerr == nil { //nolint:gosec // deployment-material path - status.OAuthClientPresent = true - status.OAuthClientFingerprint = "sha256:" + fileFingerprint(data) - if verbose { - status.OAuthClientContents = redactClientJSON(data) + if clientPath != "" { + if data, rerr := os.ReadFile(clientPath); rerr == nil { //nolint:gosec // deployment-material path + status.OAuthClientPresent = true + status.OAuthClientFingerprint = "sha256:" + fileFingerprint(data) + if verbose { + status.OAuthClientContents = redactClientJSON(data) + } } } @@ -192,6 +198,9 @@ func runShow(jsonOut, verbose bool) error { fmt.Printf("Passphrase: %s\n", status.PassphraseSource) } fmt.Printf("OAuth token: %s\n", presence(status.OAuthTokenPresent)) + if len(status.GrantedScopes) > 0 { + fmt.Printf("Granted scopes: %s\n", strings.Join(status.GrantedScopes, ", ")) + } fmt.Printf("OAuth client JSON: %s\n", status.OAuthClientPath) if status.OAuthClientPresent { fmt.Printf(" present, %s\n", status.OAuthClientFingerprint) diff --git a/internal/cmd/init/init.go b/internal/cmd/init/init.go index 2e15bf7..949030b 100644 --- a/internal/cmd/init/init.go +++ b/internal/cmd/init/init.go @@ -92,14 +92,16 @@ type initDeps struct { // JSON path (deployment material, not a secret) to reuse, plus that // sibling's name, if any. Enables seamless setup: `grw init` adopts gro's // OAuth client without a second paste. Injected so tests can stub it. - DiscoverSiblingClientJSON func() (path, sibling string, ok bool) + DiscoverSiblingClientJSON func() (path, sibling string, ok bool) + DiscoverSiblingClientJSONForProfile func(profile string) (path, sibling string, ok bool) // FS abstraction so tests can use a temp dir without env shenanigans. - GetCredentialsPath func() (string, error) - ReadFile func(path string) ([]byte, error) - WriteFile func(path string, data []byte, perm os.FileMode) error - Chmod func(path string, perm os.FileMode) error - Stat func(path string) (os.FileInfo, error) + GetCredentialsPath func() (string, error) + GetCredentialsPathForRef func(ref string) (string, error) + ReadFile func(path string) ([]byte, error) + WriteFile func(path string, data []byte, perm os.FileMode) error + Chmod func(path string, perm os.FileMode) error + Stat func(path string) (os.FileInfo, error) // Clipboard. Supported is checked first; ReadAll only called if Supported. ClipboardSupported func() bool @@ -142,8 +144,9 @@ type initDeps struct { StdinReadAll func() (string, error) // OAuth. - ExchangeAuthCode func(ctx context.Context, cfg *oauth2.Config, code string) (*oauth2.Token, error) - GetOAuthConfig func() (*oauth2.Config, error) + ExchangeAuthCode func(ctx context.Context, cfg *oauth2.Config, code string) (*oauth2.Token, error) + GetOAuthConfig func() (*oauth2.Config, error) + GetOAuthConfigForRef func(ref string) (*oauth2.Config, error) // API verifiers (one Gmail, one People). Both used during init. GmailVerify func(ctx context.Context) (string, error) // returns email @@ -176,8 +179,10 @@ type prompter interface { // defaultDeps wires up production collaborators. func defaultDeps() initDeps { return initDeps{ - View: view.New(), - DiscoverSiblingClientJSON: config.SiblingOAuthClientPath, + View: view.New(), + DiscoverSiblingClientJSONForProfile: func(profile string) (string, string, bool) { + return config.SiblingOAuthClientPathForProfile(profile) + }, // The OAuth client JSON is deployment material (§1.2): the wizard // writes it to oauth_client_path, not the legacy credentials.json. GetCredentialsPath: func() (string, error) { @@ -187,6 +192,21 @@ func defaultDeps() initDeps { } return config.ExpandPath(cfg.OAuthClientPath), nil }, + GetCredentialsPathForRef: func(ref string) (string, error) { + cfg, err := config.LoadConfigForRuntime() + if err != nil { + return "", err + } + path := cfg.OAuthClientPathForRef(ref) + if path != "" { + return path, nil + } + profile, err := config.ProfileNameForRef(ref) + if err != nil { + return "", err + } + return config.ProfileOAuthClientPath(profile) + }, ReadFile: os.ReadFile, WriteFile: os.WriteFile, Chmod: os.Chmod, @@ -206,6 +226,7 @@ func defaultDeps() initDeps { StdinReadAll: readAllStdin, ExchangeAuthCode: auth.ExchangeAuthCode, GetOAuthConfig: auth.GetOAuthConfig, + GetOAuthConfigForRef: auth.GetOAuthConfigForRef, GmailVerify: func(ctx context.Context) (string, error) { c, err := gmail.NewClient(ctx) if err != nil { @@ -418,18 +439,18 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { target = "the active profile" } - credPath, err := d.GetCredentialsPath() + credPath, err := credentialsPathForRef(d, targetRef) if err != nil { return fmt.Errorf("getting credentials path: %w", err) } // Step 1: ensure credentials.json exists. - if err := ensureCredentials(d, opts, credPath); err != nil { + if err := ensureCredentialsForRef(d, opts, credPath, targetRef); err != nil { return err } // Step 3: token resolution. - handled, err := tryExistingToken(ctx, d, opts, target) + handled, err := tryExistingTokenForRef(ctx, d, opts, target, targetRef) if err != nil { return err } @@ -438,7 +459,7 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { } // Step 4: OAuth flow. - oauthCfg, err := d.GetOAuthConfig() + oauthCfg, err := oauthConfigForRef(d, targetRef) if err != nil { return fmt.Errorf("loading OAuth config: %w", err) } @@ -490,14 +511,26 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { d.View.Success("Token saved to %s", d.GetStorageBackend()) } - // Step 5: persist granted scopes (creates config.json if missing). + // Step 5: persist the selected profile's granted scopes (creates config.yml + // if missing). cfg, cfgErr := d.LoadConfig() if cfgErr != nil { - cfg = &config.Config{} - } - cfg.GrantedScopes = config.Scopes() - if saveErr := d.SaveConfig(cfg); saveErr != nil { - d.View.Error("Warning: saving granted scopes: %v", saveErr) + d.View.Error("Warning: loading config to save granted scopes: %v", cfgErr) + } else { + if targetRef != "" { + clientPath := cfg.OAuthClientPathForRef(targetRef) + if clientPath == "" { + clientPath = credPath + } + if err := cfg.SetProfileOAuth(targetRef, config.ProfileConfig{OAuthClientPath: clientPath, GrantedScopes: config.Scopes()}); err != nil { + d.View.Error("Warning: saving granted scopes: %v", err) + } + } else { + cfg.GrantedScopes = config.Scopes() + } + if saveErr := d.SaveConfig(cfg); saveErr != nil { + d.View.Error("Warning: saving granted scopes: %v", saveErr) + } } // Step 7: verify the token works. Gmail is verified for every CLI built on @@ -610,15 +643,7 @@ func apisForScopes(scopes []string) []string { return apis } -// tryExistingToken handles the case where a token is already stored. -// Returns (handled=true, nil) if init is done; (handled=false, nil) if the -// caller should fall through to the OAuth flow; (_, err) on errors. -// -// This is the load-bearing piece for #107's stale-scope fix: a Gmail-valid -// but People-insufficient token (typical of users who upgraded gro) must -// trigger re-auth here, otherwise `gro me`'s "run gro init" message -// produces an infinite remediation loop. -func tryExistingToken(ctx context.Context, d initDeps, opts *initOptions, target string) (bool, error) { +func tryExistingTokenForRef(ctx context.Context, d initDeps, opts *initOptions, target, targetRef string) (bool, error) { if !d.HasStoredToken() { return false, nil } @@ -627,7 +652,11 @@ func tryExistingToken(ctx context.Context, d initDeps, opts *initOptions, target // regardless of --no-verify because letting --no-verify skip it would // re-open the same remediation loop #107 is trying to close. if cfg, err := d.LoadConfig(); err == nil { - if msg := auth.CheckScopesMigration(cfg.GrantedScopes); msg != "" { + granted := cfg.GrantedScopes + if targetRef != "" { + granted = cfg.GrantedScopesForRef(targetRef) + } + if msg := auth.CheckScopesMigration(granted); msg != "" { d.View.Error("Recorded scopes are stale.") d.View.Println(msg) if err := promptAndDeleteForReauth(d, opts, target); err != nil { @@ -727,16 +756,44 @@ func finishExisting(d initDeps, profile *people.Profile) error { return nil } -// ensureCredentials makes sure credentials.json exists at credPath, populating -// it from --credentials-file or the interactive wizard if needed. +func credentialsPathForRef(d initDeps, ref string) (string, error) { + if d.GetCredentialsPathForRef != nil { + return d.GetCredentialsPathForRef(ref) + } + if d.GetCredentialsPath == nil { + return "", errors.New("OAuth client path resolver is unavailable") + } + return d.GetCredentialsPath() +} + +func oauthConfigForRef(d initDeps, ref string) (*oauth2.Config, error) { + if d.GetOAuthConfigForRef != nil { + return d.GetOAuthConfigForRef(ref) + } + if d.GetOAuthConfig == nil { + return nil, errors.New("OAuth client resolver is unavailable") + } + return d.GetOAuthConfig() +} + +// ensureCredentials keeps the old three-argument test seam for callers that +// already resolved a path. Production uses ensureCredentialsForRef so imports +// are associated with the exact selected profile. func ensureCredentials(d initDeps, opts *initOptions, credPath string) error { + return ensureCredentialsForRef(d, opts, credPath, "") +} + +// ensureCredentialsForRef makes sure an OAuth client exists for targetRef, +// populating a profile-managed path from --credentials-file or the interactive +// wizard if needed. +func ensureCredentialsForRef(d initDeps, opts *initOptions, credPath, targetRef string) error { // --credentials-file flag wins. if opts.credentialsFile != "" { expanded, err := expandTilde(opts.credentialsFile) if err != nil { return err } - return importFromFile(d, expanded, credPath) + return importProfileFromFile(d, expanded, targetRef, credPath) } if _, err := d.Stat(credPath); err == nil { @@ -747,7 +804,18 @@ func ensureCredentials(d initDeps, opts *initOptions, credPath string) error { // it (deployment material, not a secret) so the user need not paste it a // second time. Only the OAuth *client* is shared here — each CLI still runs // its own consent and stores its own token in its own keyring namespace. - if d.DiscoverSiblingClientJSON != nil { + profile := "default" + if targetRef != "" { + if p, err := config.ProfileNameForRef(targetRef); err == nil { + profile = p + } + } + if d.DiscoverSiblingClientJSONForProfile != nil { + if srcPath, sibling, ok := d.DiscoverSiblingClientJSONForProfile(profile); ok { + d.View.Info("Reusing the OAuth client from %s - no need to paste it again.", sibling) + return importProfileFromFile(d, srcPath, targetRef, credPath) + } + } else if d.DiscoverSiblingClientJSON != nil && profile == "default" { if srcPath, sibling, ok := d.DiscoverSiblingClientJSON(); ok { d.View.Info("Reusing the OAuth client from %s - no need to paste it again.", sibling) return importFromFile(d, srcPath, credPath) @@ -825,7 +893,12 @@ func ensureCredentials(d initDeps, opts *initOptions, credPath string) error { return fmt.Errorf("unknown choice: %s", choice) } - if err := writeCredentials(d, credPath, blob); err != nil { + if targetRef != "" { + if err := importProfileJSON(d, blob, targetRef, credPath); err != nil { + d.View.Error("%v", err) + continue + } + } else if err := writeCredentials(d, credPath, blob); err != nil { d.View.Error("%v", err) continue } @@ -844,6 +917,79 @@ func importFromFile(d initDeps, srcPath, dstPath string) error { return writeCredentials(d, dstPath, blob) } +func importProfileFromFile(d initDeps, srcPath, targetRef, fallbackPath string) error { + blob, err := d.ReadFile(srcPath) + if err != nil { + return fmt.Errorf("reading %s: %w", srcPath, err) + } + return importProfileJSON(d, blob, targetRef, fallbackPath) +} + +// importProfileJSON validates an OAuth client and associates it with exactly +// targetRef. A selected profile never writes the active profile's client path. +func importProfileJSON(d initDeps, blob []byte, targetRef, fallbackPath string) error { + imported, err := google.ConfigFromJSON(blob, config.Scopes()...) + if err != nil { + return fmt.Errorf("invalid OAuth client JSON: %w", err) + } + if targetRef == "" { + return writeCredentials(d, fallbackPath, blob) + } + + cfg, err := d.LoadConfig() + if err != nil { + return fmt.Errorf("loading profile config: %w", err) + } + path := cfg.OAuthClientPathForRef(targetRef) + if path == "" { + path = fallbackPath + } + if path == "" { + profile, perr := config.ProfileNameForRef(targetRef) + if perr != nil { + return perr + } + if d.GetCredentialsPathForRef != nil { + path, err = d.GetCredentialsPathForRef(targetRef) + } else { + path, err = config.ProfileOAuthClientPath(profile) + } + if err != nil { + return fmt.Errorf("creating profile OAuth client path: %w", err) + } + } + + // A stored token is tied to the OAuth app. Refuse a different client ID + // rather than creating a token/client mismatch that fails later. + if d.HasStoredToken != nil && d.HasStoredToken() { + var current *oauth2.Config + if d.GetOAuthConfigForRef != nil { + current, err = d.GetOAuthConfigForRef(targetRef) + } else if d.GetOAuthConfig != nil { + current, err = d.GetOAuthConfig() + } + if err == nil && current != nil && current.ClientID != "" && current.ClientID != imported.ClientID { + return fmt.Errorf("profile %s already has a token for a different OAuth client; clear that profile or use another profile", targetRef) + } + } + if err := writeCredentials(d, path, blob); err != nil { + return err + } + state, _, err := cfg.ProfileOAuthForRef(targetRef) + if err != nil { + return err + } + state.OAuthClientPath = path + if err := cfg.SetProfileOAuth(targetRef, state); err != nil { + return err + } + if err := d.SaveConfig(cfg); err != nil { + return fmt.Errorf("saving profile OAuth client association: %w", err) + } + d.View.Success("OAuth client JSON saved for %s", targetRef) + return nil +} + // writeCredentials validates blob as OAuth client JSON and writes it to dst at // 0600. We chmod after WriteFile because os.WriteFile won't tighten an // already-existing file's permissions. diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index 1df4508..fe17e0b 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -289,6 +289,49 @@ func TestEnsureCredentialsFlagFile(t *testing.T) { } } +func TestEnsureCredentialsProfileFileBindsOnlySelectedProfile(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + const ref = "google-readonly/work" + activePath := filepath.Join(t.TempDir(), "default.json") + profilePath := filepath.Join(t.TempDir(), "oauth_clients", "work.json") + cfgPtr := &config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": {OAuthClientPath: activePath}, + }, + } + d.LoadConfig = func() (*config.Config, error) { return cfgPtr, nil } + d.SaveConfig = func(cfg *config.Config) error { cfgPtr = cfg; return nil } + d.GetCredentialsPathForRef = func(got string) (string, error) { + if got != ref { + t.Fatalf("credentials resolver ref = %q, want %q", got, ref) + } + return profilePath, nil + } + src := filepath.Join(t.TempDir(), "work-client.json") + if err := os.WriteFile(src, []byte(validOAuthJSON), 0644); err != nil { + t.Fatal(err) + } + d.Prompter = &stubPrompter{} + if err := ensureCredentialsForRef(d, &initOptions{credentialsFile: src}, profilePath, ref); err != nil { + t.Fatalf("ensureCredentialsForRef: %v", err) + } + if _, err := fs.ReadFile(profilePath); err != nil { + t.Fatalf("selected profile client was not written: %v", err) + } + if got := cfgPtr.OAuthClientPathForRef(ref); got != profilePath { + t.Fatalf("selected profile client = %q, want %q", got, profilePath) + } + if got := cfgPtr.OAuthClientPathForRef(config.DefaultCredentialRef); got != activePath { + t.Fatalf("active profile client = %q, want %q", got, activePath) + } + if len(cfgPtr.Profiles) != 2 { + t.Fatalf("profiles after import = %v, want active plus selected", cfgPtr.Profiles) + } +} + func TestEnsureCredentialsRejectsBadJSON(t *testing.T) { t.Parallel() fs := newFakeFS() diff --git a/internal/cmd/init/sibling_test.go b/internal/cmd/init/sibling_test.go index a1254ce..a754791 100644 --- a/internal/cmd/init/sibling_test.go +++ b/internal/cmd/init/sibling_test.go @@ -31,6 +31,30 @@ func TestEnsureCredentials_ReusesSiblingOAuthClient(t *testing.T) { } } +func TestEnsureCredentials_ReusesSiblingOAuthClientForSelectedProfile(t *testing.T) { + fs := newFakeFS() + d := baseDeps(t, fs) + credPath := filepath.Join(t.TempDir(), "oauth_clients", "work.json") + siblingPath := filepath.Join(t.TempDir(), "sibling-work.json") + fs.files[siblingPath] = []byte(validOAuthJSON) + var selected string + d.Prompter = &stubPrompter{} + d.DiscoverSiblingClientJSONForProfile = func(profile string) (string, string, bool) { + selected = profile + return siblingPath, "google-readonly", true + } + + if err := ensureCredentialsForRef(d, &initOptions{}, credPath, "google-readonly/work"); err != nil { + t.Fatalf("ensureCredentialsForRef: %v", err) + } + if selected != "work" { + t.Fatalf("sibling profile = %q, want work", selected) + } + if _, ok := fs.files[credPath]; !ok { + t.Fatal("expected the selected profile's sibling OAuth client to be written") + } +} + // TestEnsureCredentials_NoSiblingFallsThroughToWizard proves the discovery does // not hijack the normal path: with no sibling, the wizard still runs. func TestEnsureCredentials_NoSiblingFallsThroughToWizard(t *testing.T) { diff --git a/internal/cmd/me/me.go b/internal/cmd/me/me.go index f4673ae..6782b95 100644 --- a/internal/cmd/me/me.go +++ b/internal/cmd/me/me.go @@ -14,6 +14,7 @@ import ( "github.com/open-cli-collective/google-cli/internal/api/people" "github.com/open-cli-collective/google-cli/internal/auth" "github.com/open-cli-collective/google-cli/internal/config" + "github.com/open-cli-collective/google-cli/internal/keychain" ) // errReauth is the well-known error returned when the user must re-run `gro init`. @@ -64,9 +65,11 @@ Data comes from the People API people/me endpoint.`, func run(ctx context.Context, out, errOut io.Writer, idOnly, extended bool) error { // Loud-and-early stale-scope check (only fires when scopes were recorded). if cfg, err := config.LoadConfigForRuntime(); err == nil { - if msg := auth.CheckScopesMigration(cfg.GrantedScopes); msg != "" { - _, _ = fmt.Fprintln(errOut, msg) - return errReauth + if ref, rerr := keychain.ResolveEffectiveCredentialRef(); rerr == nil { + if msg := auth.CheckScopesMigration(cfg.GrantedScopesForRef(ref)); msg != "" { + _, _ = fmt.Fprintln(errOut, msg) + return errReauth + } } } @@ -134,5 +137,9 @@ func grantedScopes() []string { if err != nil { return nil } - return cfg.GrantedScopes + ref, err := keychain.ResolveEffectiveCredentialRef() + if err != nil { + return nil + } + return cfg.GrantedScopesForRef(ref) } diff --git a/internal/cmd/profiles/profiles_test.go b/internal/cmd/profiles/profiles_test.go index b53ad76..8246132 100644 --- a/internal/cmd/profiles/profiles_test.go +++ b/internal/cmd/profiles/profiles_test.go @@ -188,7 +188,9 @@ func TestRunList_Check(t *testing.T) { seedToken(t, "work") seedToken(t, "broken") + seen := map[string]int{} withVerify(t, func(_ context.Context, ref string) (string, error) { + seen[ref]++ switch ref { case "google-readonly/default": return "", &oauth2.RetrieveError{ @@ -217,6 +219,11 @@ func TestRunList_Check(t *testing.T) { if strings.Contains(out, "second line noise") { t.Errorf("multi-line error leaked past firstLine:\n%s", out) } + for _, ref := range []string{"google-readonly/default", "google-readonly/work", "google-readonly/broken"} { + if seen[ref] != 1 { + t.Errorf("--check verified %q %d times, want exactly once", ref, seen[ref]) + } + } // Healthy check must refresh the identity cache. if got := identitycache.Load()["work"].Email; got != "work@example.com" { t.Errorf("identity cache after check = %q, want work@example.com", got) diff --git a/internal/config/config.go b/internal/config/config.go index 4984795..64639ca 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -17,6 +17,7 @@ import ( "os" "path/filepath" + "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/cli-common/statedir" "gopkg.in/yaml.v3" ) @@ -67,13 +68,16 @@ type Config struct { // CredentialRef is the authoritative / keyring ref // (§1.3). Resolved via credstore.ParseRef; never hard-coded. CredentialRef string `yaml:"credential_ref" json:"credential_ref,omitempty"` - // OAuthClientPath is the absolute path to the OAuth client JSON - // (deployment material). Stored expanded + absolute; `~` is display-only - // via ShortenPath. An org may override the default location here. + // OAuthClientPath is the legacy active-profile path accepted while loading + // older config files. Canonical saves store it under Profiles instead. OAuthClientPath string `yaml:"oauth_client_path" json:"oauth_client_path,omitempty"` - // GrantedScopes is preserved: detects when a token's scopes drift from - // what init granted. Not a secret. + // GrantedScopes is the legacy active-profile scope record accepted during + // migration. Canonical saves store it under Profiles instead. GrantedScopes []string `yaml:"granted_scopes,omitempty" json:"granted_scopes,omitempty"` + // Profiles stores OAuth client JSON paths and granted scopes by bare, + // validated credential profile name. The legacy top-level fields above are + // accepted on load and normalized into the configured active profile. + Profiles map[string]ProfileConfig `yaml:"profiles,omitempty" json:"profiles,omitempty"` // Keyring carries the optional §1.4 explicit file-backend opt-in. Keyring KeyringConfig `yaml:"keyring,omitempty" json:"-"` @@ -85,6 +89,101 @@ type Config struct { credentialRefSource RefSource } +// ProfileConfig is the non-secret state owned by one bare credential profile. +// Access tokens remain in the keyring under the corresponding full ref. +type ProfileConfig struct { + OAuthClientPath string `yaml:"oauth_client_path,omitempty" json:"oauth_client_path,omitempty"` + GrantedScopes []string `yaml:"granted_scopes,omitempty" json:"granted_scopes,omitempty"` +} + +// ProfileNameForRef validates a full credential ref and returns its bare +// profile name. Config state is keyed by this name, never by a service- +// qualified ref, so the same profile name can be used by gro and grw. +func ProfileNameForRef(ref string) (string, error) { + _, profile, err := credstore.ParseRef(ref) + if err != nil { + return "", fmt.Errorf("invalid credential ref %q: %w", ref, err) + } + return profile, nil +} + +// ProfileOAuthForRef returns the exact profile-owned state for ref. It never +// falls back to legacy top-level fields or another profile. +func (c *Config) ProfileOAuthForRef(ref string) (ProfileConfig, bool, error) { + if c == nil { + return ProfileConfig{}, false, nil + } + profile, err := ProfileNameForRef(ref) + if err != nil { + return ProfileConfig{}, false, err + } + state, ok := c.Profiles[profile] + if !ok { + return ProfileConfig{}, false, nil + } + state.OAuthClientPath = ExpandPath(state.OAuthClientPath) + state.GrantedScopes = append([]string(nil), state.GrantedScopes...) + return state, true, nil +} + +// OAuthClientPathForRef returns the selected profile's client path, or an +// empty string when that profile has no owned client. +func (c *Config) OAuthClientPathForRef(ref string) string { + state, ok, err := c.ProfileOAuthForRef(ref) + if err != nil || !ok { + return "" + } + return state.OAuthClientPath +} + +// GrantedScopesForRef returns only the selected profile's recorded scopes. +// An unconfigured profile returns nil, even when legacy top-level scopes exist +// for the active profile. +func (c *Config) GrantedScopesForRef(ref string) []string { + state, ok, err := c.ProfileOAuthForRef(ref) + if err != nil || !ok { + return nil + } + return state.GrantedScopes +} + +// SetProfileOAuth stores exact profile-owned state after validating the bare +// profile name from ref. Paths are expanded before persistence. +func (c *Config) SetProfileOAuth(ref string, state ProfileConfig) error { + profile, err := ProfileNameForRef(ref) + if err != nil { + return err + } + if c.Profiles == nil { + c.Profiles = make(map[string]ProfileConfig) + } + state.OAuthClientPath = ExpandPath(state.OAuthClientPath) + state.GrantedScopes = append([]string(nil), state.GrantedScopes...) + c.Profiles[profile] = state + return nil +} + +// ProfileOAuthClientPath returns the managed path for a new profile. The +// caller still needs to import/validate the JSON before associating it. +func ProfileOAuthClientPath(profile string) (string, error) { + if _, err := credstore.FormatRef(serviceName(), profile); err != nil { + return "", fmt.Errorf("invalid profile %q: %w", profile, err) + } + dir, err := GetConfigDir() + if err != nil { + return "", err + } + return filepath.Join(dir, "oauth_clients", profile+".json"), nil +} + +func serviceName() string { + service, _, err := credstore.ParseRef(DefaultCredentialRef) + if err != nil { + return "" + } + return service +} + // RefSource identifies where the resolved CredentialRef came from, so auth // errors and `config show` can attribute the active profile to its source // instead of leaving the user to guess which of flag/env/config selected it. @@ -354,7 +453,9 @@ func LoadConfig() (*Config, error) { return nil, relErr } - cfg.applyDefaults() + if err := cfg.applyDefaults(); err != nil { + return nil, err + } return cfg, relErr } @@ -378,20 +479,88 @@ func loadLegacyJSON(cfg *Config) error { return nil } -func (c *Config) applyDefaults() { +func (c *Config) applyDefaults() error { if c.CredentialRef == "" { c.CredentialRef = DefaultCredentialRef c.credentialRefSource = RefSourceDefault } else { c.credentialRefSource = RefSourceConfig } - if c.OAuthClientPath == "" { - if p, err := DefaultOAuthClientPath(); err == nil { - c.OAuthClientPath = p + + // Validate and normalize profile-owned state first. A malformed profile key + // must fail closed instead of silently becoming another account's state. + for profile, state := range c.Profiles { + if _, err := credstore.FormatRef(serviceName(), profile); err != nil { + return fmt.Errorf("invalid profile %q in config: %w", profile, err) } - } else { + state.OAuthClientPath = ExpandPath(state.OAuthClientPath) + state.GrantedScopes = append([]string(nil), state.GrantedScopes...) + c.Profiles[profile] = state + } + + // Legacy top-level client/scopes are attached only to the configured active + // profile. A new profile never inherits them. Divergent mixed state fails + // loudly so a save cannot silently discard user-authored values. + active, err := ProfileNameForRef(c.CredentialRef) + if err != nil { + return err + } + legacy := ProfileConfig{ + OAuthClientPath: ExpandPath(c.OAuthClientPath), + GrantedScopes: append([]string(nil), c.GrantedScopes...), + } + if legacy.OAuthClientPath != "" || c.GrantedScopes != nil { + state, ok := c.Profiles[active] + if ok { + if legacy.OAuthClientPath != "" && state.OAuthClientPath != "" && + !oauthClientPathEquivalent(legacy.OAuthClientPath, state.OAuthClientPath) { + return fmt.Errorf("profile %q has divergent legacy and profile OAuth client paths", active) + } + if c.GrantedScopes != nil && state.GrantedScopes != nil && + !scopesEqual(legacy.GrantedScopes, state.GrantedScopes) { + return fmt.Errorf("profile %q has divergent legacy and profile granted scopes", active) + } + if state.OAuthClientPath == "" { + state.OAuthClientPath = legacy.OAuthClientPath + } + if state.GrantedScopes == nil && c.GrantedScopes != nil { + state.GrantedScopes = append([]string(nil), c.GrantedScopes...) + } + c.Profiles[active] = state + } else { + if c.Profiles == nil { + c.Profiles = make(map[string]ProfileConfig) + } + c.Profiles[active] = legacy + } + } + if c.OAuthClientPath != "" { c.OAuthClientPath = ExpandPath(c.OAuthClientPath) } + return nil +} + +func oauthClientPathEquivalent(a, b string) bool { + return ExpandPath(a) == ExpandPath(b) +} + +func scopesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + seen := make(map[string]int, len(a)) + for _, s := range a { + seen[s]++ + } + for _, s := range b { + seen[s]-- + } + for _, n := range seen { + if n != 0 { + return false + } + } + return true } // SaveConfig writes config.yml at 0600 under a 0700 directory using an atomic @@ -409,9 +578,50 @@ func SaveConfig(cfg *Config) error { // caller's *Config (a caller inspecting OAuthClientPath after SaveConfig // would otherwise observe an unexpectedly rewritten value). out := *cfg - if out.OAuthClientPath != "" { - out.OAuthClientPath = ExpandPath(out.OAuthClientPath) + out.Profiles = cloneProfiles(cfg.Profiles) + for profile := range out.Profiles { + if _, perr := credstore.FormatRef(serviceName(), profile); perr != nil { + return fmt.Errorf("invalid profile %q in config: %w", profile, perr) + } + } + if out.CredentialRef == "" { + out.CredentialRef = DefaultCredentialRef } + if profile, perr := ProfileNameForRef(out.CredentialRef); perr == nil { + legacy := ProfileConfig{ + OAuthClientPath: ExpandPath(out.OAuthClientPath), + GrantedScopes: append([]string(nil), out.GrantedScopes...), + } + if legacy.OAuthClientPath != "" || out.GrantedScopes != nil { + state, ok := out.Profiles[profile] + if !ok { + if out.Profiles == nil { + out.Profiles = make(map[string]ProfileConfig) + } + out.Profiles[profile] = legacy + } else { + if legacy.OAuthClientPath != "" && state.OAuthClientPath != "" && + !oauthClientPathEquivalent(legacy.OAuthClientPath, state.OAuthClientPath) { + return fmt.Errorf("profile %q has divergent legacy and profile OAuth client paths", profile) + } + if out.GrantedScopes != nil && state.GrantedScopes != nil && + !scopesEqual(out.GrantedScopes, state.GrantedScopes) { + return fmt.Errorf("profile %q has divergent legacy and profile granted scopes", profile) + } + if state.OAuthClientPath == "" { + state.OAuthClientPath = legacy.OAuthClientPath + } + if state.GrantedScopes == nil && out.GrantedScopes != nil { + state.GrantedScopes = append([]string(nil), out.GrantedScopes...) + } + out.Profiles[profile] = state + } + } + } + // Legacy fields are load-time compatibility only. Persist canonical + // profile-owned state so the next save cannot reintroduce a global fallback. + out.OAuthClientPath = "" + out.GrantedScopes = nil data, err := yaml.Marshal(&out) if err != nil { return err @@ -442,3 +652,16 @@ func SaveConfig(cfg *Config) error { } return nil } + +func cloneProfiles(in map[string]ProfileConfig) map[string]ProfileConfig { + if in == nil { + return nil + } + out := make(map[string]ProfileConfig, len(in)) + for profile, state := range in { + state.OAuthClientPath = ExpandPath(state.OAuthClientPath) + state.GrantedScopes = append([]string(nil), state.GrantedScopes...) + out[profile] = state + } + return out +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 02b0358..881ff96 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -243,6 +243,60 @@ func TestLoadConfig(t *testing.T) { }) } +func TestLegacyOAuthStateAttachesOnlyToActiveProfile(t *testing.T) { + hermeticConfig(t) + dir, err := GetConfigDir() + if err != nil { + t.Fatal(err) + } + data := "credential_ref: google-readonly/default\n" + + "oauth_client_path: /tmp/legacy-client.json\n" + + "granted_scopes:\n - legacy-scope\n" + + "profiles:\n" + + " work:\n" + + " oauth_client_path: /tmp/work-client.json\n" + + " granted_scopes:\n" + + " - work-scope\n" + if err := os.WriteFile(filepath.Join(dir, ConfigFileYAML), []byte(data), TokenPerm); err != nil { + t.Fatal(err) + } + cfg, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + if got := cfg.OAuthClientPathForRef("google-readonly/default"); got != "/tmp/legacy-client.json" { + t.Fatalf("active client = %q, want legacy client", got) + } + if got := cfg.OAuthClientPathForRef("google-readonly/work"); got != "/tmp/work-client.json" { + t.Fatalf("work client = %q, want work client", got) + } + if got := cfg.OAuthClientPathForRef("google-readonly/other"); got != "" { + t.Fatalf("unconfigured client = %q, want no fallback", got) + } + if got := cfg.GrantedScopesForRef("google-readonly/work"); len(got) != 1 || got[0] != "work-scope" { + t.Fatalf("work scopes = %v, want [work-scope]", got) + } +} + +func TestMixedLegacyAndProfileOAuthStateDivergesLoudly(t *testing.T) { + hermeticConfig(t) + dir, err := GetConfigDir() + if err != nil { + t.Fatal(err) + } + data := "credential_ref: google-readonly/default\n" + + "oauth_client_path: /tmp/legacy-client.json\n" + + "profiles:\n" + + " default:\n" + + " oauth_client_path: /tmp/new-client.json\n" + if err := os.WriteFile(filepath.Join(dir, ConfigFileYAML), []byte(data), TokenPerm); err != nil { + t.Fatal(err) + } + if _, err := LoadConfig(); err == nil || !strings.Contains(err.Error(), "divergent") { + t.Fatalf("LoadConfig error = %v, want divergent mixed-state error", err) + } +} + func TestSaveConfig(t *testing.T) { t.Run("saves config to file", func(t *testing.T) { hermeticConfig(t) @@ -310,6 +364,43 @@ func TestSaveConfig(t *testing.T) { } } }) + + t.Run("writes profile state without legacy top-level fallback", func(t *testing.T) { + hermeticConfig(t) + cfg := &Config{ + CredentialRef: DefaultCredentialRef, + OAuthClientPath: "/tmp/legacy-client.json", + GrantedScopes: []string{"scope-a"}, + } + if err := SaveConfig(cfg); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(filepath.Join(mustConfigDir(t), ConfigFileYAML)) + if err != nil { + t.Fatal(err) + } + for _, line := range strings.Split(string(data), "\n") { + if line == "oauth_client_path: /tmp/legacy-client.json" || line == "granted_scopes:" { + t.Fatalf("canonical config retained legacy top-level state:\n%s", data) + } + } + loaded, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + if got := loaded.OAuthClientPathForRef(DefaultCredentialRef); got != "/tmp/legacy-client.json" { + t.Fatalf("canonical profile client = %q", got) + } + }) +} + +func mustConfigDir(t *testing.T) string { + t.Helper() + dir, err := GetConfigDir() + if err != nil { + t.Fatal(err) + } + return dir } func TestCacheDirResolvers(t *testing.T) { diff --git a/internal/config/identity.go b/internal/config/identity.go index 1548d1c..9708c8d 100644 --- a/internal/config/identity.go +++ b/internal/config/identity.go @@ -1,10 +1,13 @@ package config import ( + "encoding/json" "os" "path/filepath" + "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/cli-common/statedir" + "gopkg.in/yaml.v3" ) // Identity carries the per-CLI values that were previously compile-time @@ -83,19 +86,76 @@ func Register(id Identity) { // the first sibling whose oauth_client.json exists, along with that sibling's // DirName. ok is false when no sibling has one. func SiblingOAuthClientPath() (path, siblingDirName string, ok bool) { + return SiblingOAuthClientPathForProfile("default") +} + +// SiblingOAuthClientPathForProfile finds an OAuth client explicitly owned by +// the same bare profile in a sibling CLI. Legacy top-level client state is +// eligible only when that sibling's configured active profile matches profile; +// another sibling profile is never a fallback. +func SiblingOAuthClientPathForProfile(profile string) (path, siblingDirName string, ok bool) { + if profile == "" { + profile = "default" + } for _, name := range siblingDirNames { dir, err := (statedir.Scope{Name: name}).ConfigDir() if err != nil { continue } - p := filepath.Join(dir, OAuthClientFile) - if info, err := os.Stat(p); err == nil && !info.IsDir() { + if p, found := siblingProfileClient(dir, profile); found { return p, name, true } } return "", "", false } +func siblingProfileClient(dir, profile string) (string, bool) { + path := filepath.Join(dir, ConfigFileYAML) + data, err := os.ReadFile(path) //nolint:gosec // sibling config path is resolved from registered identity + if err != nil { + if !os.IsNotExist(err) { + return "", false + } + // A fresh/legacy sibling with no config can only represent its default + // profile, and only at the historical shared client path. + if profile != "default" { + return "", false + } + p := filepath.Join(dir, OAuthClientFile) + info, serr := os.Stat(p) + return p, serr == nil && !info.IsDir() + } + + var raw struct { + CredentialRef string `yaml:"credential_ref" json:"credential_ref"` + OAuthClientPath string `yaml:"oauth_client_path" json:"oauth_client_path"` + Profiles map[string]ProfileConfig `yaml:"profiles" json:"profiles"` + } + if err := yaml.Unmarshal(data, &raw); err != nil { + if err := json.Unmarshal(data, &raw); err != nil { + return "", false + } + } + if state, ok := raw.Profiles[profile]; ok && state.OAuthClientPath != "" { + p := ExpandPath(state.OAuthClientPath) + if info, serr := os.Stat(p); serr == nil && !info.IsDir() { + return p, true + } + } + active := "default" + if raw.CredentialRef != "" { + if _, p, perr := credstore.ParseRef(raw.CredentialRef); perr == nil { + active = p + } + } + if active != profile || raw.OAuthClientPath == "" { + return "", false + } + p := ExpandPath(raw.OAuthClientPath) + info, serr := os.Stat(p) + return p, serr == nil && !info.IsDir() +} + // RegisterForTest registers a canonical identity for internal package tests // (and for any test helper that needs a valid identity without // caring about the specific CLI). It mirrors the historical google-readonly diff --git a/internal/config/relocate.go b/internal/config/relocate.go index 48d2a35..928cfa3 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -206,15 +206,80 @@ func configsMaterialEqual(a, b Config, oldDir, newDir string) bool { if !reflect.DeepEqual(a.Keyring, b.Keyring) { return false } - if !slicesEqualSorted(a.GrantedScopes, b.GrantedScopes) { + return profileConfigsMaterialEqual(a, b, oldDir, newDir) +} + +// profileConfigsMaterialEqual compares canonical profile-owned state while +// treating legacy top-level fields as belonging only to each config's active +// profile. This keeps relocation compatible with the one-time schema upgrade +// without allowing one profile's values to mask another's divergence. +func profileConfigsMaterialEqual(a, b Config, oldDir, newDir string) bool { + if legacyProfileConflict(a) || legacyProfileConflict(b) { return false } - if !oauthClientPathEquiv(a.OAuthClientPath, b.OAuthClientPath, oldDir, newDir) { + left := canonicalProfiles(a) + right := canonicalProfiles(b) + if len(left) != len(right) { return false } + for profile, aState := range left { + bState, ok := right[profile] + if !ok || !slicesEqualSorted(aState.GrantedScopes, bState.GrantedScopes) { + return false + } + if !profileOAuthClientPathEquiv(profile, aState.OAuthClientPath, bState.OAuthClientPath, oldDir, newDir) { + return false + } + } return true } +func legacyProfileConflict(c Config) bool { + if c.CredentialRef == "" { + return false + } + profile, err := ProfileNameForRef(c.CredentialRef) + if err != nil { + return true + } + state, ok := c.Profiles[profile] + if !ok { + return false + } + if c.OAuthClientPath != "" && state.OAuthClientPath != "" && + !oauthClientPathEquivalent(c.OAuthClientPath, state.OAuthClientPath) { + return true + } + return c.GrantedScopes != nil && state.GrantedScopes != nil && + !slicesEqualSorted(c.GrantedScopes, state.GrantedScopes) +} + +func canonicalProfiles(c Config) map[string]ProfileConfig { + out := cloneProfiles(c.Profiles) + if c.CredentialRef == "" { + return out + } + profile, err := ProfileNameForRef(c.CredentialRef) + if err != nil { + return out + } + if c.OAuthClientPath == "" && c.GrantedScopes == nil { + return out + } + if out == nil { + out = make(map[string]ProfileConfig) + } + state := out[profile] + if state.OAuthClientPath == "" { + state.OAuthClientPath = c.OAuthClientPath + } + if state.GrantedScopes == nil && c.GrantedScopes != nil { + state.GrantedScopes = append([]string(nil), c.GrantedScopes...) + } + out[profile] = state + return out +} + // oauthClientPathEquiv treats "both empty", "both equal to their own dir's // default oauth_client.json", and "both literally equal" as equivalent. Any // other combination — including one side at its default and the other at a @@ -235,6 +300,16 @@ func oauthClientPathEquiv(aPath, bPath, aDir, bDir string) bool { return aIsDefault && bIsDefault } +func profileOAuthClientPathEquiv(profile, aPath, bPath, aDir, bDir string) bool { + if oauthClientPathEquiv(aPath, bPath, aDir, bDir) { + return true + } + aPath = ExpandPath(aPath) + bPath = ExpandPath(bPath) + return aPath == filepath.Join(aDir, "oauth_clients", profile+".json") && + bPath == filepath.Join(bDir, "oauth_clients", profile+".json") +} + func slicesEqualSorted(a, b []string) bool { if len(a) != len(b) { return false @@ -269,6 +344,11 @@ func ApplyConfigRelocation(r SharedRelocation) error { } for _, e := range entries { if e.IsDir() { + if e.Name() == "oauth_clients" { + if err := copyOAuthClients(filepath.Join(r.OldPath, e.Name()), filepath.Join(r.NewPath, e.Name())); err != nil { + return fmt.Errorf("copying OAuth client directory: %w", err) + } + } continue // pre-B2b cache subdir etc.; not part of config relocation } if e.Name() == TokenFile { @@ -286,6 +366,30 @@ func ApplyConfigRelocation(r SharedRelocation) error { return nil } +func copyOAuthClients(srcDir, dstDir string) error { + entries, err := os.ReadDir(srcDir) + if err != nil { + return err + } + if err := os.MkdirAll(dstDir, DirPerm); err != nil { + return err + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + src := filepath.Join(srcDir, entry.Name()) + dst := filepath.Join(dstDir, entry.Name()) + if fileExists(dst) { + continue + } + if err := copyFileAtomic(src, dst); err != nil { + return err + } + } + return nil +} + func copyFileAtomic(src, dst string) error { in, err := os.Open(src) //nolint:gosec // path from old config dir if err != nil { diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go index 5e37535..4e22aba 100644 --- a/internal/config/relocate_test.go +++ b/internal/config/relocate_test.go @@ -276,6 +276,58 @@ func TestRelocate_OAuthClientPath_ExplicitNonDefaultDivergence_FailsLoud(t *test } } +func TestRelocate_ProfileOwnedOAuthState(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + oldCfg := "credential_ref: google-readonly/work\nprofiles:\n default:\n oauth_client_path: " + filepath.Join(oldDir, "oauth_clients", "default.json") + "\n granted_scopes:\n - default-scope\n work:\n oauth_client_path: " + filepath.Join(oldDir, "oauth_clients", "work.json") + "\n granted_scopes:\n - work-scope\n" + newCfg := "credential_ref: google-readonly/work\nprofiles:\n default:\n oauth_client_path: " + filepath.Join(newDir, "oauth_clients", "default.json") + "\n granted_scopes:\n - default-scope\n work:\n oauth_client_path: " + filepath.Join(newDir, "oauth_clients", "work.json") + "\n granted_scopes:\n - work-scope\n" + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte(oldCfg), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte(newCfg), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if err != nil || r.Kind != relocBothEqual { + t.Fatalf("profile maps should compare equal across relocated dirs: kind=%v err=%v", r.Kind, err) + } + + divergent := strings.Replace(newCfg, "- work-scope", "- other-scope", 1) + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte(divergent), 0o600); err != nil { + t.Fatal(err) + } + if _, err := detectAt(t, oldDir, newDir); !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("profile scope divergence must fail loud, got %v", err) + } +} + +func TestApplyConfigRelocation_CopiesProfileOAuthClients(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(filepath.Join(oldDir, "oauth_clients"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte("credential_ref: google-readonly/work\nprofiles:\n work:\n oauth_client_path: "+filepath.Join(oldDir, "oauth_clients", "work.json")+"\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, "oauth_clients", "work.json"), []byte("client"), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if err != nil || !r.CopyNeeded { + t.Fatalf("old-only profile state should request relocation: copy=%v err=%v", r.CopyNeeded, err) + } + if err := ApplyConfigRelocation(r); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(newDir, "oauth_clients", "work.json")); err != nil { + t.Fatalf("profile OAuth client was not copied: %v", err) + } +} + func TestRelocate_OldOnlyConfigJSON_TriggersCopy(t *testing.T) { // A pre-MON-5371 install on macOS may have only legacy config.json (not // yet promoted to config.yml). Detection must still classify as diff --git a/internal/e2e/profile_scoped_test.go b/internal/e2e/profile_scoped_test.go new file mode 100644 index 0000000..98686e2 --- /dev/null +++ b/internal/e2e/profile_scoped_test.go @@ -0,0 +1,171 @@ +package e2e + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + clicache "github.com/open-cli-collective/cli-common/cache" + + "github.com/open-cli-collective/google-cli/internal/cache" +) + +func TestBuiltCLIProfileStateIsHermetic(t *testing.T) { + root := repositoryRoot(t) + tmp := t.TempDir() + configHome := filepath.Join(tmp, "config") + cacheHome := filepath.Join(tmp, "cache") + if err := os.MkdirAll(filepath.Join(configHome, "google-readonly"), 0o700); err != nil { + t.Fatal(err) + } + defaultClient := `{"installed":{"client_id":"default-client.apps.googleusercontent.com","project_id":"p","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://oauth2.googleapis.com/token","client_secret":"default-secret","redirect_uris":["http://localhost"]}}` + workClient := `{"installed":{"client_id":"work-client.apps.googleusercontent.com","project_id":"p","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://oauth2.googleapis.com/token","client_secret":"work-secret","redirect_uris":["http://localhost"]}}` + defaultPath := filepath.Join(configHome, "google-readonly", "default.json") + workPath := filepath.Join(configHome, "google-readonly", "work.json") + if err := os.WriteFile(defaultPath, []byte(defaultClient), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(workPath, []byte(workClient), 0o600); err != nil { + t.Fatal(err) + } + configYAML := "credential_ref: google-readonly/default\nprofiles:\n" + + " default:\n" + + " oauth_client_path: " + defaultPath + "\n" + + " granted_scopes:\n" + + " - default-scope\n" + + " work:\n" + + " oauth_client_path: " + workPath + "\n" + + " granted_scopes:\n" + + " - work-scope\n" + if err := os.WriteFile(filepath.Join(configHome, "google-readonly", "config.yml"), []byte(configYAML), 0o600); err != nil { + t.Fatal(err) + } + + bin := filepath.Join(tmp, "gro") + build := exec.Command("go", "build", "-tags", "keyring_no1password,keyring_nopassage", "-o", bin, "./cmd/gro") + build.Dir = root + if output, err := build.CombinedOutput(); err != nil { + t.Fatalf("build gro: %v\n%s", err, output) + } + runtimeEnv := hermeticEnv(configHome, cacheHome, tmp) + + defaultStatus := showCLI(t, bin, runtimeEnv, "--backend=file", "config", "show", "--json") + workStatus := showCLI(t, bin, runtimeEnv, "--backend=file", "--profile", "work", "config", "show", "--json") + if defaultStatus.OAuthClientFingerprint == "" || workStatus.OAuthClientFingerprint == "" || defaultStatus.OAuthClientFingerprint == workStatus.OAuthClientFingerprint { + t.Fatalf("profile fingerprints = %q and %q, want distinct non-empty values", defaultStatus.OAuthClientFingerprint, workStatus.OAuthClientFingerprint) + } + if got := defaultStatus.GrantedScopes; len(got) != 1 || got[0] != "default-scope" { + t.Fatalf("default scopes = %v, want [default-scope]", got) + } + if got := workStatus.GrantedScopes; len(got) != 1 || got[0] != "work-scope" { + t.Fatalf("work scopes = %v, want [work-scope]", got) + } + unconfigured := showCLI(t, bin, runtimeEnv, "--backend=file", "--profile", "other", "config", "show", "--json") + if unconfigured.OAuthClientPresent || unconfigured.OAuthClientFingerprint != "" || unconfigured.OAuthClientPath != "" { + t.Fatalf("unconfigured profile exposed active client: %+v", unconfigured) + } + + cacheBase := cacheHome + switch runtime.GOOS { + case "darwin": + cacheBase = filepath.Join(tmp, "home", "Library", "Caches") + case "windows": + cacheBase = filepath.Join(tmp, "home", "AppData", "Local") + } + rootCache := filepath.Join(cacheBase, "google-readonly") + if err := clicache.WriteResource(clicache.Locator{Root: rootCache, InstanceKey: "default"}, "drives", "24h", []*cache.CachedDrive{{ID: "default", Name: "Default"}}); err != nil { + t.Fatal(err) + } + defaultFresh := refreshStatus(t, bin, runtimeEnv, "--backend=file", "--profile", "default", "refresh", "--status", "--json") + workMiss := refreshStatus(t, bin, runtimeEnv, "--backend=file", "--profile", "work", "refresh", "--status", "--json") + if defaultFresh != "fresh" || workMiss != "uninitialized" { + t.Fatalf("cache status = default %q, work %q; want fresh/uninitialized", defaultFresh, workMiss) + } +} + +type status struct { + OAuthClientPath string `json:"oauth_client_path"` + OAuthClientPresent bool `json:"oauth_client_present"` + OAuthClientFingerprint string `json:"oauth_client_fingerprint"` + GrantedScopes []string `json:"granted_scopes"` +} + +func showCLI(t *testing.T, bin string, env []string, args ...string) status { + t.Helper() + cmd := exec.Command(bin, args...) + cmd.Env = env + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("gro %s: %v\n%s", strings.Join(args, " "), err, output) + } + var got status + if err := json.Unmarshal(output, &got); err != nil { + t.Fatalf("parse config show output: %v\n%s", err, output) + } + return got +} + +func refreshStatus(t *testing.T, bin string, env []string, args ...string) string { + t.Helper() + cmd := exec.Command(bin, args...) + cmd.Env = env + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("gro %s: %v\n%s", strings.Join(args, " "), err, output) + } + var got struct { + Resources []struct { + Status string `json:"status"` + } `json:"resources"` + } + if err := json.Unmarshal(output, &got); err != nil { + t.Fatalf("parse refresh output: %v\n%s", err, output) + } + if len(got.Resources) != 1 { + t.Fatalf("refresh resources = %d, want 1", len(got.Resources)) + } + return got.Resources[0].Status +} + +func hermeticEnv(configHome, cacheHome, tmp string) []string { + env := os.Environ() + values := map[string]string{ + "HOME": filepath.Join(tmp, "home"), + "XDG_CONFIG_HOME": configHome, + "XDG_CACHE_HOME": cacheHome, + "XDG_DATA_HOME": filepath.Join(tmp, "data"), + "XDG_STATE_HOME": filepath.Join(tmp, "state"), + "GOOGLE_READONLY_KEYRING_BACKEND": "file", + "GOOGLE_READONLY_KEYRING_PASSPHRASE": "built-cli-test-passphrase", + "GRO_TEST_DISABLE_LEGACY_KEYCHAIN_SCAN": "1", + "GRO_TEST_DISABLE_LEGACY_SECRETTOOL_SCAN": "1", + } + for key, value := range values { + setEnv(&env, key, value) + } + return env +} + +func setEnv(env *[]string, key, value string) { + prefix := key + "=" + for i, entry := range *env { + if strings.HasPrefix(entry, prefix) { + (*env)[i] = prefix + value + return + } + } + *env = append(*env, prefix+value) +} + +func repositoryRoot(t *testing.T) string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + return filepath.Clean(filepath.Join(filepath.Dir(file), "..", "..")) +} diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index eec59e5..f50e5f2 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -129,6 +129,23 @@ func effectiveRef(configRef string) (ref string, source config.RefSource, overri return configRef, "", false } +// ResolveEffectiveCredentialRef applies the same flag > environment > config +// precedence used by Open without opening the keyring or migrating anything. +// Account-derived state (OAuth clients, scopes, and caches) uses this resolver +// so it cannot accidentally follow the active config while a command targets a +// different profile. +func ResolveEffectiveCredentialRef() (string, error) { + cfg, err := config.LoadConfigForRuntime() + if err != nil { + return "", err + } + ref, _, _ := effectiveRef(cfg.CredentialRef) + if _, _, err := credstore.ParseRef(ref); err != nil { + return "", fmt.Errorf("invalid credential_ref %q: %w", ref, err) + } + return ref, nil +} + // OpenRef opens a store against an explicit ref instead of config.yml's // credential_ref — used by the refresh // persister. An empty ref falls back to the configured/default ref. diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index 5d6dbae..59efeb9 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "runtime" "strings" @@ -52,7 +53,7 @@ type candidate struct { func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error { // Deployment material first (independent of the secret; never blocks the // token path and never fails loud for a non-secret). - if err := migrateOAuthClientJSON(cfg); err != nil { + if err := migrateOAuthClientJSON(cfg, s.ref); err != nil { return err } @@ -363,7 +364,14 @@ func secureDelete(path string) error { // branch that proceeds deletes the legacy file, so a later run sees it absent // and is silent; the only non-deleting branch (both invalid) is a hard, // recoverable error the user must resolve, so it cannot loop unnoticed. -func migrateOAuthClientJSON(cfg *config.Config) error { +func migrateOAuthClientJSON(cfg *config.Config, refs ...string) error { + ref := cfg.CredentialRef + if len(refs) > 0 && refs[0] != "" { + ref = refs[0] + } + if _, err := config.ProfileNameForRef(ref); err != nil { + return err + } legacyPath, err := config.GetCredentialsPath() if err != nil { return err @@ -373,12 +381,27 @@ func migrateOAuthClientJSON(cfg *config.Config) error { return nil // legacy absent → nothing to do (steady state) } - target := config.ExpandPath(cfg.OAuthClientPath) + target := cfg.OAuthClientPathForRef(ref) + if target == "" { + // Direct unit callers may still construct a legacy Config by hand; keep + // that seam working while loaded config always has legacy state attached + // to the active profile by config.LoadConfig. + target = config.ExpandPath(cfg.OAuthClientPath) + } if target == "" { if target, err = config.DefaultOAuthClientPath(); err != nil { return err } - cfg.OAuthClientPath = target + } + state, _, err := cfg.ProfileOAuthForRef(ref) + if err != nil { + return err + } + if state.OAuthClientPath == "" { + state.OAuthClientPath = target + } + if err := cfg.SetProfileOAuth(ref, state); err != nil { + return err } legacyValid := validClientJSON(legacyData) @@ -387,6 +410,12 @@ func migrateOAuthClientJSON(cfg *config.Config) error { targetValid := targetExists && validClientJSON(targetData) switch { + case filepath.Clean(target) == filepath.Clean(legacyPath): + // The user explicitly pointed the legacy field at this existing file. + // Schema migration keeps that deployment material in place. + if !legacyValid { + return fmt.Errorf("OAuth client JSON at %s is invalid", config.ShortenPath(target)) + } case targetExists && targetValid: // A usable client JSON is already installed; the legacy copy is // redundant deployment material. Remove it (idempotence). @@ -418,7 +447,7 @@ func migrateOAuthClientJSON(cfg *config.Config) error { config.ShortenPath(legacyPath), config.ShortenPath(target)) } - cfg.OAuthClientPath = target + cfg.OAuthClientPath = target // compatibility for direct callers; SaveConfig canonicalizes it return nil } From 9b70fa26df4f2011c76b6f6774bac3a9d69353c2 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 22 Sep 2026 06:52:56 -0400 Subject: [PATCH 2/6] fix: heal profile client associations --- internal/cache/cache.go | 15 +++------ internal/cmd/drive/main_test.go | 13 ++++++++ internal/cmd/init/init.go | 37 +++++++++++++++++++++ internal/cmd/init/init_test.go | 54 +++++++++++++++++++++++++++++++ internal/cmd/refresh/main_test.go | 13 ++++++++ internal/config/config.go | 2 +- internal/config/config_test.go | 2 +- 7 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 internal/cmd/drive/main_test.go create mode 100644 internal/cmd/refresh/main_test.go diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 6e1576d..b4fbea3 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -4,9 +4,9 @@ // state at os.UserCacheDir()/google-readonly (via statedir.Cache). Writes are // atomic via cli-common/cache's temp+rename envelope; TTL is hard-coded per // resource (no user-configurable cache_ttl_hours — §4.4); reads classify a -// version/identity mismatch as a miss so schema bumps self-heal. The pre-B2b -// "/cache/" relocation is retained for installs that pre-date the -// B2b cache move. +// version/identity mismatch as a miss so schema bumps self-heal. Legacy +// unscoped cache data is intentionally treated as a miss because it cannot be +// safely attributed to a profile. package cache import ( @@ -54,14 +54,7 @@ type Cache struct { func New() (*Cache, error) { ref, err := keychain.ResolveEffectiveCredentialRef() if err != nil { - // Package-level tests that construct infrastructure before registering an - // identity have no effective ref; keep their cache hermetic while real - // binaries always register one before command execution. - if config.DefaultCredentialRef == "" { - ref = "test/default" - } else { - return nil, err - } + return nil, err } _, profile, err := credstore.ParseRef(ref) if err != nil { diff --git a/internal/cmd/drive/main_test.go b/internal/cmd/drive/main_test.go new file mode 100644 index 0000000..6a049ef --- /dev/null +++ b/internal/cmd/drive/main_test.go @@ -0,0 +1,13 @@ +package drive + +import ( + "os" + "testing" + + "github.com/open-cli-collective/google-cli/internal/config" +) + +func TestMain(m *testing.M) { + config.RegisterForTest() + os.Exit(m.Run()) +} diff --git a/internal/cmd/init/init.go b/internal/cmd/init/init.go index 949030b..3eeeb06 100644 --- a/internal/cmd/init/init.go +++ b/internal/cmd/init/init.go @@ -797,6 +797,9 @@ func ensureCredentialsForRef(d initDeps, opts *initOptions, credPath, targetRef } if _, err := d.Stat(credPath); err == nil { + if targetRef != "" { + return associateExistingProfileClient(d, credPath, targetRef) + } return nil } @@ -908,6 +911,40 @@ func ensureCredentialsForRef(d initDeps, opts *initOptions, credPath, targetRef return errors.New("could not obtain valid credentials.json after 3 attempts") } +// associateExistingProfileClient heals a selected profile whose client JSON +// was written successfully but whose config association was not (for example, +// a prior SaveConfig failure or a pre-provisioned managed path). It validates +// the existing bytes before recording that exact path for targetRef, and never +// consults another profile's state. +func associateExistingProfileClient(d initDeps, credPath, targetRef string) error { + cfg, err := d.LoadConfig() + if err != nil { + return fmt.Errorf("loading profile config: %w", err) + } + state, associated, err := cfg.ProfileOAuthForRef(targetRef) + if err != nil { + return err + } + if associated && state.OAuthClientPath != "" { + return nil + } + blob, err := d.ReadFile(credPath) + if err != nil { + return fmt.Errorf("reading existing OAuth client JSON %s: %w", credPath, err) + } + if _, err := google.ConfigFromJSON(blob, config.Scopes()...); err != nil { + return fmt.Errorf("invalid OAuth client JSON at %s: %w", credPath, err) + } + state.OAuthClientPath = credPath + if err := cfg.SetProfileOAuth(targetRef, state); err != nil { + return err + } + if err := d.SaveConfig(cfg); err != nil { + return fmt.Errorf("saving profile OAuth client association: %w", err) + } + return nil +} + // importFromFile reads, validates, and writes credentials.json from a path. func importFromFile(d initDeps, srcPath, dstPath string) error { blob, err := d.ReadFile(srcPath) diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index fe17e0b..f438ade 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -332,6 +332,60 @@ func TestEnsureCredentialsProfileFileBindsOnlySelectedProfile(t *testing.T) { } } +func TestEnsureCredentialsHealsExistingSelectedProfileClient(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + const ref = "google-readonly/work" + activePath := filepath.Join(t.TempDir(), "default.json") + profilePath := filepath.Join(t.TempDir(), "oauth_clients", "work.json") + fs.files[profilePath] = []byte(validOAuthJSON) + cfgPtr := &config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": {OAuthClientPath: activePath}, + }, + } + d.LoadConfig = func() (*config.Config, error) { return cfgPtr, nil } + d.SaveConfig = func(cfg *config.Config) error { cfgPtr = cfg; return nil } + d.GetCredentialsPathForRef = func(got string) (string, error) { + if got != ref { + t.Fatalf("credentials resolver ref = %q, want %q", got, ref) + } + return profilePath, nil + } + d.GetOAuthConfigForRef = func(got string) (*oauth2.Config, error) { + if got != ref { + t.Fatalf("OAuth resolver ref = %q, want %q", got, ref) + } + if cfgPtr.OAuthClientPathForRef(got) == "" { + return nil, errors.New("selected profile has no client association") + } + return &oauth2.Config{ClientID: "selected-profile-client"}, nil + } + d.Prompter = &stubPrompter{} + + if err := ensureCredentialsForRef(d, &initOptions{}, profilePath, ref); err != nil { + t.Fatalf("ensureCredentialsForRef: %v", err) + } + if got := cfgPtr.OAuthClientPathForRef(ref); got != profilePath { + t.Fatalf("selected profile client = %q, want %q", got, profilePath) + } + if got := cfgPtr.OAuthClientPathForRef(config.DefaultCredentialRef); got != activePath { + t.Fatalf("active profile client = %q, want %q", got, activePath) + } + resolved, err := oauthConfigForRef(d, ref) + if err != nil { + t.Fatalf("oauthConfigForRef after healing: %v", err) + } + if resolved.ClientID != "selected-profile-client" { + t.Fatalf("resolved selected client ID = %q", resolved.ClientID) + } + if len(d.Prompter.(*stubPrompter).calls) != 0 { + t.Fatalf("pre-provisioned client should not start the wizard: calls=%v", d.Prompter.(*stubPrompter).calls) + } +} + func TestEnsureCredentialsRejectsBadJSON(t *testing.T) { t.Parallel() fs := newFakeFS() diff --git a/internal/cmd/refresh/main_test.go b/internal/cmd/refresh/main_test.go new file mode 100644 index 0000000..d25a308 --- /dev/null +++ b/internal/cmd/refresh/main_test.go @@ -0,0 +1,13 @@ +package refresh + +import ( + "os" + "testing" + + "github.com/open-cli-collective/google-cli/internal/config" +) + +func TestMain(m *testing.M) { + config.RegisterForTest() + os.Exit(m.Run()) +} diff --git a/internal/config/config.go b/internal/config/config.go index 64639ca..437a6f3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -70,7 +70,7 @@ type Config struct { CredentialRef string `yaml:"credential_ref" json:"credential_ref,omitempty"` // OAuthClientPath is the legacy active-profile path accepted while loading // older config files. Canonical saves store it under Profiles instead. - OAuthClientPath string `yaml:"oauth_client_path" json:"oauth_client_path,omitempty"` + OAuthClientPath string `yaml:"oauth_client_path,omitempty" json:"oauth_client_path,omitempty"` // GrantedScopes is the legacy active-profile scope record accepted during // migration. Canonical saves store it under Profiles instead. GrantedScopes []string `yaml:"granted_scopes,omitempty" json:"granted_scopes,omitempty"` diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 881ff96..2cd751d 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -380,7 +380,7 @@ func TestSaveConfig(t *testing.T) { t.Fatal(err) } for _, line := range strings.Split(string(data), "\n") { - if line == "oauth_client_path: /tmp/legacy-client.json" || line == "granted_scopes:" { + if strings.HasPrefix(line, "oauth_client_path:") || strings.HasPrefix(line, "granted_scopes:") { t.Fatalf("canonical config retained legacy top-level state:\n%s", data) } } From d10dc0d5c56997d01fe1cd2bdc9e4e4b6c4612d2 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 22 Sep 2026 07:01:34 -0400 Subject: [PATCH 3/6] test: cover profile-scoped state boundaries --- internal/cmd/init/init_test.go | 76 ++++++++++++++++++++++++++++ internal/cmd/me/me_test.go | 67 +++++++++++++++++++++++++ internal/config/identity_test.go | 80 ++++++++++++++++++++++++++++++ internal/keychain/keychain_test.go | 24 +++++++++ 4 files changed, 247 insertions(+) create mode 100644 internal/config/identity_test.go diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index f438ade..97eb2c5 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -9,6 +9,7 @@ import ( "net/url" "os" "path/filepath" + "reflect" "strings" "testing" "time" @@ -639,6 +640,81 @@ func TestRunWithFreshSetupSavesScopesNoTTLPrompt(t *testing.T) { } } +func TestRunWithWorkProfileFreshOAuthKeepsDefaultState(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + const workRef = "google-readonly/work" + defaultPath := filepath.Join(t.TempDir(), "default-client.json") + workPath := filepath.Join(t.TempDir(), "oauth_clients", "work.json") + defaultScopes := []string{"default-only-scope"} + cfgPtr := &config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": { + OAuthClientPath: defaultPath, + GrantedScopes: defaultScopes, + }, + }, + } + d.LoadConfig = func() (*config.Config, error) { return cfgPtr, nil } + d.SaveConfig = func(cfg *config.Config) error { + cfgPtr = cfg + return nil + } + d.DescribeTarget = func() (string, config.RefSource, string) { + return workRef, config.RefSourceFlag, "" + } + d.GetCredentialsPathForRef = func(ref string) (string, error) { + if ref != workRef { + t.Fatalf("credentials resolver ref = %q, want %q", ref, workRef) + } + return workPath, nil + } + d.GetOAuthConfigForRef = func(ref string) (*oauth2.Config, error) { + if ref != workRef { + t.Fatalf("OAuth resolver ref = %q, want %q", ref, workRef) + } + if got := cfgPtr.OAuthClientPathForRef(ref); got != workPath { + t.Fatalf("OAuth resolver saw work client %q, want %q", got, workPath) + } + return &oauth2.Config{ClientID: "work-client"}, nil + } + d.ExchangeAuthCode = func(_ context.Context, cfg *oauth2.Config, code string) (*oauth2.Token, error) { + if cfg.ClientID != "work-client" { + t.Fatalf("exchanged with client %q, want work-client", cfg.ClientID) + } + if code != "ABC" { + t.Fatalf("authorization code = %q, want ABC", code) + } + return &oauth2.Token{AccessToken: "work-token"}, nil + } + src := filepath.Join(t.TempDir(), "downloaded.json") + if err := os.WriteFile(src, []byte(validOAuthJSON), 0644); err != nil { + t.Fatal(err) + } + d.Prompter = &stubPrompter{redirectURL: "http://localhost/?code=ABC"} + + if err := runWith(context.Background(), d, &initOptions{credentialsFile: src, noBrowser: true}); err != nil { + t.Fatalf("runWith: %v", err) + } + if got, err := fs.ReadFile(workPath); err != nil || string(got) != validOAuthJSON { + t.Fatalf("work client = %q, err=%v; want imported client JSON", got, err) + } + if got := cfgPtr.OAuthClientPathForRef(config.DefaultCredentialRef); got != defaultPath { + t.Fatalf("default client = %q, want unchanged %q", got, defaultPath) + } + if got := cfgPtr.GrantedScopesForRef(config.DefaultCredentialRef); !reflect.DeepEqual(got, defaultScopes) { + t.Fatalf("default scopes = %v, want unchanged %v", got, defaultScopes) + } + if got := cfgPtr.OAuthClientPathForRef(workRef); got != workPath { + t.Fatalf("work client = %q, want %q", got, workPath) + } + if got := cfgPtr.GrantedScopesForRef(workRef); !reflect.DeepEqual(got, config.Scopes()) { + t.Fatalf("work scopes = %v, want config.Scopes() %v", got, config.Scopes()) + } +} + func TestRunWithExpiredTokenPromptsReauth(t *testing.T) { t.Parallel() fs := newFakeFS() diff --git a/internal/cmd/me/me_test.go b/internal/cmd/me/me_test.go index d315974..f890e7e 100644 --- a/internal/cmd/me/me_test.go +++ b/internal/cmd/me/me_test.go @@ -13,6 +13,7 @@ import ( "github.com/open-cli-collective/google-cli/internal/api/people" "github.com/open-cli-collective/google-cli/internal/config" + "github.com/open-cli-collective/google-cli/internal/keychain" ) // mockPeopleClient is a stub for the exported PeopleClient interface. @@ -397,6 +398,72 @@ func TestRunStaleRecordedScopesTriggersReauthMessage(t *testing.T) { } } +func TestRunProfileUsesSelectedScopeRecordBeforeClientCreation(t *testing.T) { + // Not Parallel: mutates env + ClientFactory + the --profile selection seam. + withConfigDir(t) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": {GrantedScopes: config.Scopes()}, + "work": {GrantedScopes: []string{"work-only-scope"}}, + }, + }); err != nil { + t.Fatal(err) + } + keychain.SetCredentialRefOverride("google-readonly/work", true) + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + t.Fatal("People client must not be created when the selected profile scopes are stale") + return nil, nil + }, + }) + + var out, errOut bytes.Buffer + err := run(context.Background(), &out, &errOut, false, false) + if !errors.Is(err, errReauth) { + t.Fatalf("expected errReauth for stale work scopes, got %v", err) + } + if !strings.Contains(errOut.String(), "gro init") { + t.Fatalf("expected re-auth guidance, got %q", errOut.String()) + } +} + +func TestRunProfileExtendedRendersSelectedScopes(t *testing.T) { + // Not Parallel: mutates env + ClientFactory + the --profile selection seam. + withConfigDir(t) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + defaultScopes := append(config.Scopes(), "default-only-scope") + workScopes := append(config.Scopes(), "work-only-scope") + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + Profiles: map[string]config.ProfileConfig{ + "default": {GrantedScopes: defaultScopes}, + "work": {GrantedScopes: workScopes}, + }, + }); err != nil { + t.Fatal(err) + } + keychain.SetCredentialRefOverride("google-readonly/work", true) + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + return &people.Profile{ResourceName: "people/c1", DisplayName: "Ada", PrimaryEmail: "ada@example.com"}, nil + }, + }) + + var out bytes.Buffer + if err := run(context.Background(), &out, &bytes.Buffer{}, false, true); err != nil { + t.Fatalf("run --profile work --extended: %v", err) + } + got := out.String() + if !strings.Contains(got, "work-only-scope") { + t.Fatalf("extended output must include work scopes, got %q", got) + } + if strings.Contains(got, "default-only-scope") { + t.Fatalf("extended output must not include default scopes, got %q", got) + } +} + func TestRunMissingConfigDoesNotShortCircuit(t *testing.T) { // Not Parallel: mutates env + ClientFactory. withConfigDir(t) diff --git a/internal/config/identity_test.go b/internal/config/identity_test.go new file mode 100644 index 0000000..c459c64 --- /dev/null +++ b/internal/config/identity_test.go @@ -0,0 +1,80 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/open-cli-collective/cli-common/statedir" + "github.com/open-cli-collective/cli-common/statedirtest" + "gopkg.in/yaml.v3" +) + +func TestSiblingOAuthClientPathForProfileIsProfileScoped(t *testing.T) { + statedirtest.Hermetic(t) + t.Cleanup(RegisterForTest) + Register(Identity{ + DirName: "google-readonly", + DefaultRef: "google-readonly/default", + ProductName: "gro", + Scopes: Scopes(), + ScopeDescriptions: ScopeDescriptions(), + SiblingDirNames: []string{"sibling-full", "sibling-default"}, + }) + + fullDir, err := (statedir.Scope{Name: "sibling-full"}).ConfigDir() + if err != nil { + t.Fatal(err) + } + defaultOnlyDir, err := (statedir.Scope{Name: "sibling-default"}).ConfigDir() + if err != nil { + t.Fatal(err) + } + defaultPath := filepath.Join(fullDir, "oauth_clients", "default.json") + workPath := filepath.Join(fullDir, "oauth_clients", "work.json") + if err := os.MkdirAll(filepath.Dir(defaultPath), DirPerm); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(defaultOnlyDir, DirPerm); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(defaultPath, []byte("full-default-client"), TokenPerm); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(workPath, []byte("full-work-client"), TokenPerm); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(defaultOnlyDir, OAuthClientFile), []byte("default-only-client"), TokenPerm); err != nil { + t.Fatal(err) + } + configData, err := yaml.Marshal(struct { + CredentialRef string `yaml:"credential_ref"` + Profiles map[string]ProfileConfig `yaml:"profiles"` + }{ + CredentialRef: "sibling-full/default", + Profiles: map[string]ProfileConfig{ + "default": {OAuthClientPath: defaultPath}, + "work": {OAuthClientPath: workPath}, + }, + }) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(fullDir, ConfigFileYAML), configData, TokenPerm); err != nil { + t.Fatal(err) + } + + if got, sibling, ok := SiblingOAuthClientPathForProfile("default"); !ok || got != defaultPath || sibling != "sibling-full" { + t.Fatalf("default sibling = (%q, %q, %v), want (%q, sibling-full, true)", got, sibling, ok, defaultPath) + } + if got, sibling, ok := SiblingOAuthClientPathForProfile("work"); !ok || got != workPath || sibling != "sibling-full" { + t.Fatalf("work sibling = (%q, %q, %v), want (%q, sibling-full, true)", got, sibling, ok, workPath) + } + + if err := os.Remove(workPath); err != nil { + t.Fatal(err) + } + if got, sibling, ok := SiblingOAuthClientPathForProfile("work"); ok { + t.Fatalf("work selected a fallback client after its own client disappeared: (%q, %q)", got, sibling) + } +} diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index 0e30657..3f9f656 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -309,6 +309,30 @@ func TestMigrateOAuthClientJSON(t *testing.T) { } }) + t.Run("records target association for a non-default active profile", func(t *testing.T) { + credtest.Setup(t) + dir := credtest.ConfigDir(t) + legacy := filepath.Join(dir, "credentials.json") + target := filepath.Join(dir, "oauth_clients", "work.json") + if err := os.MkdirAll(filepath.Dir(target), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(legacy, []byte(validClientJSONFixture), 0o600); err != nil { + t.Fatal(err) + } + activeRef := "google-readonly/work" + cfg := &config.Config{CredentialRef: activeRef, OAuthClientPath: target} + if err := migrateOAuthClientJSON(cfg); err != nil { + t.Fatalf("migrate: %v", err) + } + if got := cfg.OAuthClientPathForRef(activeRef); got != target { + t.Fatalf("active profile client = %q, want %q", got, target) + } + if got := cfg.OAuthClientPathForRef(config.DefaultCredentialRef); got != "" { + t.Fatalf("default profile client = %q, want no association", got) + } + }) + t.Run("target valid present: legacy removed", func(t *testing.T) { credtest.Setup(t) dir := credtest.ConfigDir(t) From 975d0e19236fbbb295320dde97c08864253907d8 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 22 Sep 2026 07:02:04 -0400 Subject: [PATCH 4/6] test: assert profile scope check precedes client creation --- internal/cmd/me/me_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/cmd/me/me_test.go b/internal/cmd/me/me_test.go index f890e7e..e27e8ac 100644 --- a/internal/cmd/me/me_test.go +++ b/internal/cmd/me/me_test.go @@ -412,12 +412,12 @@ func TestRunProfileUsesSelectedScopeRecordBeforeClientCreation(t *testing.T) { t.Fatal(err) } keychain.SetCredentialRefOverride("google-readonly/work", true) - withMockClient(t, &mockPeopleClient{ - GetMeFunc: func(_ context.Context) (*people.Profile, error) { - t.Fatal("People client must not be created when the selected profile scopes are stale") - return nil, nil - }, - }) + origClientFactory := ClientFactory + ClientFactory = func(context.Context) (PeopleClient, error) { + t.Fatal("People client must not be created when the selected profile scopes are stale") + return nil, nil + } + t.Cleanup(func() { ClientFactory = origClientFactory }) var out, errOut bytes.Buffer err := run(context.Background(), &out, &errOut, false, false) From 93c06dc9e803234d01921a9f0ca820bfcaedbac5 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 22 Sep 2026 07:09:44 -0400 Subject: [PATCH 5/6] fix: persist migrated profile client association --- internal/keychain/keychain_test.go | 46 ++++++++++++++++++++++++++++++ internal/keychain/migrate.go | 33 ++++++++++++++------- 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index 3f9f656..f745540 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -391,3 +391,49 @@ func TestMigrateOAuthClientJSON(t *testing.T) { } }) } + +func TestOpenMigrationPersistsProfileClientAssociationWithoutToken(t *testing.T) { + credtest.Setup(t) + dir := credtest.ConfigDir(t) + activeRef := "google-readonly/work" + configPath, err := config.GetConfigPath() + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(configPath, []byte("credential_ref: "+activeRef+"\n"), config.TokenPerm); err != nil { + t.Fatal(err) + } + legacyPath := filepath.Join(dir, config.CredentialsFile) + if err := os.WriteFile(legacyPath, []byte(validClientJSONFixture), config.TokenPerm); err != nil { + t.Fatal(err) + } + targetPath, err := config.DefaultOAuthClientPath() + if err != nil { + t.Fatal(err) + } + + st, err := Open() + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := st.Close(); err != nil { + t.Fatal(err) + } + + loaded, err := config.LoadConfig() + if err != nil { + t.Fatalf("reload config: %v", err) + } + if got := loaded.OAuthClientPathForRef(activeRef); got != targetPath { + t.Fatalf("reloaded work client = %q, want %q", got, targetPath) + } + if got := loaded.OAuthClientPathForRef(config.DefaultCredentialRef); got != "" { + t.Fatalf("reloaded default client = %q, want no association", got) + } + if _, err := os.Stat(targetPath); err != nil { + t.Fatalf("migrated OAuth client missing at %s: %v", targetPath, err) + } + if _, err := os.Stat(legacyPath); !os.IsNotExist(err) { + t.Fatalf("legacy credentials.json should be removed, stat err=%v", err) + } +} diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index 59efeb9..a78c901 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -53,18 +53,29 @@ type candidate struct { func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error { // Deployment material first (independent of the secret; never blocks the // token path and never fails loud for a non-secret). + clientPathBefore := cfg.OAuthClientPathForRef(s.ref) if err := migrateOAuthClientJSON(cfg, s.ref); err != nil { return err } + clientPathChanged := clientPathBefore != cfg.OAuthClientPathForRef(s.ref) // Promote a legacy config.json to config.yml so §2.3's "read transparently // once" actually holds even when there is no legacy token to migrate. - if err := promoteLegacyConfigJSON(cfg); err != nil { + promotedConfig, err := promoteLegacyConfigJSON(cfg) + if err != nil { return err } cands := discover() if len(cands) == 0 { + // migrateOAuthClientJSON can have associated a moved legacy client with + // the selected profile even when there is no legacy token. Persist that + // association, unless promotion already saved the same canonical config. + if clientPathChanged && !promotedConfig { + if err := config.SaveConfig(cfg); err != nil { + return fmt.Errorf("migration succeeded but writing config.yml failed: %w", err) + } + } return nil // nothing legacy on disk/keychain — the steady state } @@ -457,38 +468,38 @@ func migrateOAuthClientJSON(cfg *config.Config, refs ...string) error { // config.yml exists the loader never reads config.json again and a second run // is a silent no-op. config.json is non-secret (refs/paths/ttl/scopes), so a // plain os.Remove is sufficient. -func promoteLegacyConfigJSON(cfg *config.Config) error { +func promoteLegacyConfigJSON(cfg *config.Config) (bool, error) { ymlPath, err := config.GetConfigPath() if err != nil { - return err + return false, err } switch _, serr := os.Stat(ymlPath); { case serr == nil: - return nil // config.yml already present — nothing to promote + return false, nil // config.yml already present — nothing to promote case !os.IsNotExist(serr): - return serr + return false, serr } jsonPath, err := config.LegacyConfigJSONPath() if err != nil { - return err + return false, err } switch _, serr := os.Stat(jsonPath); { case os.IsNotExist(serr): - return nil // no legacy config.json — fresh-install steady state + return false, nil // no legacy config.json — fresh-install steady state case serr != nil: - return serr + return false, serr } if err := config.SaveConfig(cfg); err != nil { - return fmt.Errorf("promote legacy config.json to config.yml: %w", err) + return false, fmt.Errorf("promote legacy config.json to config.yml: %w", err) } if err := os.Remove(jsonPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("remove legacy config.json after promotion: %w", err) + return false, fmt.Errorf("remove legacy config.json after promotion: %w", err) } fmt.Fprintf(os.Stderr, "Migrated %s -> %s (non-secret config).\n", config.ShortenPath(jsonPath), config.ShortenPath(ymlPath)) - return nil + return true, nil } // validClientJSON reports whether data parses as a Google OAuth client From 2c25ab46a92cbb1737f5d39a5e0fc35fea12e833 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 22 Sep 2026 07:18:12 -0400 Subject: [PATCH 6/6] fix: make OAuth client migration retryable --- internal/keychain/keychain_test.go | 54 ++++++++++++++++++++ internal/keychain/migrate.go | 82 +++++++++++++++++++++--------- 2 files changed, 112 insertions(+), 24 deletions(-) diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index f745540..1644509 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -437,3 +437,57 @@ func TestOpenMigrationPersistsProfileClientAssociationWithoutToken(t *testing.T) t.Fatalf("legacy credentials.json should be removed, stat err=%v", err) } } + +func TestOpenMigrationRetriesAfterConfigSaveFailure(t *testing.T) { + credtest.Setup(t) + dir := credtest.ConfigDir(t) + activeRef := "google-readonly/work" + legacyPath := filepath.Join(dir, config.CredentialsFile) + if err := os.WriteFile(legacyPath, []byte(validClientJSONFixture), config.TokenPerm); err != nil { + t.Fatal(err) + } + configPath, err := config.GetConfigPath() + if err != nil { + t.Fatal(err) + } + if err := os.Mkdir(configPath, config.DirPerm); err != nil { + t.Fatal(err) + } + + first := &config.Config{CredentialRef: activeRef} + if _, err := openWith(first, false, true); err == nil { + t.Fatal("expected config save failure") + } + if _, err := os.Stat(legacyPath); err != nil { + t.Fatalf("legacy credentials.json must remain after save failure: %v", err) + } + targetPath, err := config.DefaultOAuthClientPath() + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(targetPath); err != nil { + t.Fatalf("copied OAuth client missing after failed save: %v", err) + } + + if err := os.Remove(configPath); err != nil { + t.Fatal(err) + } + second := &config.Config{CredentialRef: activeRef} + st, err := openWith(second, false, true) + if err != nil { + t.Fatalf("retry migration: %v", err) + } + if err := st.Close(); err != nil { + t.Fatal(err) + } + loaded, err := config.LoadConfig() + if err != nil { + t.Fatalf("reload config after retry: %v", err) + } + if got := loaded.OAuthClientPathForRef(activeRef); got != targetPath { + t.Fatalf("reloaded work client = %q, want %q", got, targetPath) + } + if _, err := os.Stat(legacyPath); !os.IsNotExist(err) { + t.Fatalf("legacy credentials.json should be removed after successful retry, stat err=%v", err) + } +} diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index a78c901..64c64c1 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -54,7 +54,8 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error // Deployment material first (independent of the secret; never blocks the // token path and never fails loud for a non-secret). clientPathBefore := cfg.OAuthClientPathForRef(s.ref) - if err := migrateOAuthClientJSON(cfg, s.ref); err != nil { + removeLegacyClient, err := migrateOAuthClientJSONDeferred(cfg, s.ref) + if err != nil { return err } clientPathChanged := clientPathBefore != cfg.OAuthClientPathForRef(s.ref) @@ -76,6 +77,11 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error return fmt.Errorf("migration succeeded but writing config.yml failed: %w", err) } } + if removeLegacyClient != nil { + if err := removeLegacyClient(); err != nil { + return err + } + } return nil // nothing legacy on disk/keychain — the steady state } @@ -112,6 +118,11 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error if err := config.SaveConfig(cfg); err != nil { return fmt.Errorf("migration succeeded but writing config.yml failed: %w", err) } + if removeLegacyClient != nil { + if err := removeLegacyClient(); err != nil { + return err + } + } return nil } @@ -371,25 +382,41 @@ func secureDelete(path string) error { } // migrateOAuthClientJSON relocates the legacy credentials.json (deployment -// material, §1.2) to cfg.OAuthClientPath. Idempotent by construction: every -// branch that proceeds deletes the legacy file, so a later run sees it absent -// and is silent; the only non-deleting branch (both invalid) is a hard, -// recoverable error the user must resolve, so it cannot loop unnoticed. +// material, §1.2) to cfg.OAuthClientPath. It retains the historical immediate +// cleanup behavior for direct callers; the Open migration path uses the +// deferred variant below so config persistence succeeds before the legacy +// source is removed. func migrateOAuthClientJSON(cfg *config.Config, refs ...string) error { + removeLegacy, err := migrateOAuthClientJSONDeferred(cfg, refs...) + if err != nil { + return err + } + if removeLegacy == nil { + return nil + } + return removeLegacy() +} + +// migrateOAuthClientJSONDeferred prepares the OAuth client migration and +// returns a cleanup that removes the legacy source. Deferring that cleanup is +// important: if the following canonical config save fails, retaining +// credentials.json lets the next process retry and re-associate the selected +// profile instead of leaving only an unreferenced client file behind. +func migrateOAuthClientJSONDeferred(cfg *config.Config, refs ...string) (func() error, error) { ref := cfg.CredentialRef if len(refs) > 0 && refs[0] != "" { ref = refs[0] } if _, err := config.ProfileNameForRef(ref); err != nil { - return err + return nil, err } legacyPath, err := config.GetCredentialsPath() if err != nil { - return err + return nil, err } legacyData, lerr := os.ReadFile(legacyPath) //nolint:gosec // path from config dir if lerr != nil { - return nil // legacy absent → nothing to do (steady state) + return nil, nil // legacy absent → steady state } target := cfg.OAuthClientPathForRef(ref) @@ -401,43 +428,47 @@ func migrateOAuthClientJSON(cfg *config.Config, refs ...string) error { } if target == "" { if target, err = config.DefaultOAuthClientPath(); err != nil { - return err + return nil, err } } state, _, err := cfg.ProfileOAuthForRef(ref) if err != nil { - return err + return nil, err } if state.OAuthClientPath == "" { state.OAuthClientPath = target } if err := cfg.SetProfileOAuth(ref, state); err != nil { - return err + return nil, err } legacyValid := validClientJSON(legacyData) targetData, terr := os.ReadFile(target) //nolint:gosec // path from config dir targetExists := terr == nil targetValid := targetExists && validClientJSON(targetData) + var removeLegacy func() error switch { case filepath.Clean(target) == filepath.Clean(legacyPath): // The user explicitly pointed the legacy field at this existing file. // Schema migration keeps that deployment material in place. if !legacyValid { - return fmt.Errorf("OAuth client JSON at %s is invalid", config.ShortenPath(target)) + return nil, fmt.Errorf("OAuth client JSON at %s is invalid", config.ShortenPath(target)) } case targetExists && targetValid: // A usable client JSON is already installed; the legacy copy is - // redundant deployment material. Remove it (idempotence). - if rmErr := os.Remove(legacyPath); rmErr != nil && !os.IsNotExist(rmErr) { - return fmt.Errorf("remove superseded legacy credentials.json: %w", rmErr) + // redundant deployment material. Remove it after config persistence. + removeLegacy = func() error { + if rmErr := os.Remove(legacyPath); rmErr != nil && !os.IsNotExist(rmErr) { + return fmt.Errorf("remove superseded legacy credentials.json: %w", rmErr) + } + fmt.Fprintf(os.Stderr, "Removed superseded legacy credentials.json (OAuth client JSON already present at %s).\n", + config.ShortenPath(target)) + return nil } - fmt.Fprintf(os.Stderr, "Removed superseded legacy credentials.json (OAuth client JSON already present at %s).\n", - config.ShortenPath(target)) case !legacyValid && (!targetExists || !targetValid): // Both unusable: do not delete anything. Loud, lossless, recoverable. - return fmt.Errorf( + return nil, fmt.Errorf( "OAuth client JSON migration cannot proceed: legacy %s (sha256:%s) and target %s (%s) are both missing or invalid; install a valid OAuth client JSON at %s", config.ShortenPath(legacyPath), fingerprint(legacyData), config.ShortenPath(target), targetStateStr(targetExists, targetData), @@ -449,17 +480,20 @@ func migrateOAuthClientJSON(cfg *config.Config, refs ...string) error { // not attacker/network input. Same threat model as the G304 reads // above; the OAuth client JSON is deployment material (§1.2). if werr := os.WriteFile(target, legacyData, config.OutputFilePerm); werr != nil { //nolint:gosec // G703: config-derived path, user's own deployment material - return fmt.Errorf("write OAuth client JSON %s: %w", config.ShortenPath(target), werr) + return nil, fmt.Errorf("write OAuth client JSON %s: %w", config.ShortenPath(target), werr) } - if rmErr := os.Remove(legacyPath); rmErr != nil && !os.IsNotExist(rmErr) { - return fmt.Errorf("remove legacy credentials.json after copy: %w", rmErr) + removeLegacy = func() error { + if rmErr := os.Remove(legacyPath); rmErr != nil && !os.IsNotExist(rmErr) { + return fmt.Errorf("remove legacy credentials.json after copy: %w", rmErr) + } + fmt.Fprintf(os.Stderr, "Migrated OAuth client JSON: %s -> %s (deployment material, not a secret).\n", + config.ShortenPath(legacyPath), config.ShortenPath(target)) + return nil } - fmt.Fprintf(os.Stderr, "Migrated OAuth client JSON: %s -> %s (deployment material, not a secret).\n", - config.ShortenPath(legacyPath), config.ShortenPath(target)) } cfg.OAuthClientPath = target // compatibility for direct callers; SaveConfig canonicalizes it - return nil + return removeLegacy, nil } // promoteLegacyConfigJSON rewrites a legacy config.json as config.yml and