From 75da336c4cad49817dfbf8d4002c0d3ad99544aa Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 15 Sep 2026 10:43:04 -0400 Subject: [PATCH 1/5] feat: add global profile selection and rename --- README.md | 26 ++ internal/app/gro/credref_wire_test.go | 98 ++++++ internal/app/grw/credref_wire_test.go | 109 ++++++ internal/app/grw/main_test.go | 13 + internal/cmd/init/init.go | 41 +-- internal/cmd/init/init_test.go | 38 +-- internal/cmd/profiles/profiles.go | 112 ++++++- internal/cmd/profiles/profiles_test.go | 330 ++++++++++++++++++- internal/cmd/setcred/setcred.go | 22 +- internal/cmd/setcred/setcred_test.go | 19 ++ internal/identitycache/identitycache.go | 31 ++ internal/identitycache/identitycache_test.go | 51 +++ internal/keychain/keychain.go | 56 ++++ internal/keychain/profiles_test.go | 107 ++++++ internal/rootutil/rootutil.go | 74 ++++- internal/rootutil/rootutil_test.go | 114 +++++++ 16 files changed, 1152 insertions(+), 89 deletions(-) create mode 100644 internal/app/grw/credref_wire_test.go create mode 100644 internal/app/grw/main_test.go create mode 100644 internal/rootutil/rootutil_test.go diff --git a/README.md b/README.md index 3b39ed5..b6a1715 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,32 @@ 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). +## Profiles + +Each tool has its own profile namespace. Use a bare profile name with the global +`--profile` shorthand, or pass the full credential reference with `--ref`: + +```bash +gro --profile work mail list +grw --profile work calendar today +gro --ref google-readonly/work mail list +``` + +The selector precedence is explicit flag (`--profile` or `--ref`), credential +reference environment variable, saved `credential_ref`, then the built-in +`default` profile. `--profile` and `--ref` cannot be used together. To add an +account without changing the active profile, run `gro --profile work init` (or +the equivalent `grw` command). Inspect and manage profiles with: + +```bash +gro profiles list +gro profiles rename old-name new-name +``` + +Renaming moves the stored credentials without re-authentication, updates the +saved active profile when necessary, and refuses a destination that already +has credentials. + ## Documentation - [Development](docs/development.md) diff --git a/internal/app/gro/credref_wire_test.go b/internal/app/gro/credref_wire_test.go index 466d451..ea485bc 100644 --- a/internal/app/gro/credref_wire_test.go +++ b/internal/app/gro/credref_wire_test.go @@ -1,11 +1,15 @@ package gro import ( + "path/filepath" "strings" "testing" "github.com/spf13/cobra" + initcmd "github.com/open-cli-collective/google-cli/internal/cmd/init" + "github.com/open-cli-collective/google-cli/internal/cmd/setcred" + "github.com/open-cli-collective/google-cli/internal/credtest" "github.com/open-cli-collective/google-cli/internal/keychain" "github.com/open-cli-collective/google-cli/internal/rootutil" ) @@ -116,3 +120,97 @@ func TestCredentialRef_SetCredentialShadowsPersistent(t *testing.T) { t.Errorf("read command --%s = %p, want canonical %p (unexpected shadow)", rootutil.CredentialRefFlagName, got, canonical) } } + +func selectorTestRoot() *cobra.Command { + var verbose, noColor bool + root := &cobra.Command{ + Use: "gro", + PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { + return rootutil.ApplyGlobalFlags(cmd, verbose, noColor) + }, + } + rootutil.AddGlobalFlags(root, &verbose, &noColor) + root.AddCommand(initcmd.NewCommand()) + root.AddCommand(setcred.NewCmd()) + return root +} + +func TestProfileFlagInheritedByInitInBothFlagOrders(t *testing.T) { + for _, tc := range []struct { + name string + args func(string) []string + }{ + {name: "before command", args: func(path string) []string { + return []string{"--profile", "work", "init", "--credentials-file", path} + }}, + {name: "after command", args: func(path string) []string { + return []string{"init", "--profile", "work", "--credentials-file", path} + }}, + } { + t.Run(tc.name, func(t *testing.T) { + credtest.Setup(t) + t.Setenv(keychain.CredentialRefEnvVar(), "") + root := selectorTestRoot() + root.SetArgs(tc.args(filepath.Join(t.TempDir(), "missing.json"))) + if err := root.Execute(); err == nil { + t.Fatal("init should fail for the intentionally missing client file") + } + if got, set := keychain.GetCredentialRefOverride(); !set || got != "google-readonly/work" { + t.Fatalf("selector after init path = (%q, %v), want google-readonly/work", got, set) + } + }) + } +} + +func TestProfileFlagInheritedBySetCredentialTargetsNamedProfile(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {name: "before command", args: []string{"--profile", "work", "set-credential", "--key", "oauth_token", "--stdin"}}, + {name: "after command", args: []string{"set-credential", "--profile", "work", "--key", "oauth_token", "--stdin"}}, + } { + t.Run(tc.name, func(t *testing.T) { + credtest.Setup(t) + t.Setenv(keychain.CredentialRefEnvVar(), "") + root := selectorTestRoot() + root.SetIn(strings.NewReader(`{"access_token":"profile-token","refresh_token":"refresh"}`)) + root.SetArgs(tc.args) + if err := root.Execute(); err != nil { + t.Fatalf("set-credential: %v", err) + } + st, err := keychain.OpenRef("google-readonly/work") + if err != nil { + t.Fatal(err) + } + tok, err := st.Token() + _ = st.Close() + if err != nil || tok.AccessToken != "profile-token" { + t.Fatalf("named profile token = %+v, err=%v", tok, err) + } + assertNoTokenAtRef(t, "google-readonly/default") + }) + } +} + +func TestProfileAndSetCredentialRefAreMutuallyExclusive(t *testing.T) { + credtest.Setup(t) + root := selectorTestRoot() + root.SetIn(strings.NewReader(`{"access_token":"profile-token"}`)) + root.SetArgs([]string{"--profile", "work", "set-credential", "--ref", "google-readonly/other", "--key", "oauth_token", "--stdin"}) + if err := root.Execute(); err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Fatalf("profile/local --ref conflict = %v, want mutual-exclusion error", err) + } +} + +func assertNoTokenAtRef(t *testing.T, ref string) { + t.Helper() + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatal(err) + } + defer func() { _ = st.Close() }() + if has, err := st.HasToken(); err != nil || has { + t.Fatalf("%s token presence = (%v, %v), want (false, nil)", ref, has, err) + } +} diff --git a/internal/app/grw/credref_wire_test.go b/internal/app/grw/credref_wire_test.go new file mode 100644 index 0000000..2129d8c --- /dev/null +++ b/internal/app/grw/credref_wire_test.go @@ -0,0 +1,109 @@ +package grw + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + + initcmd "github.com/open-cli-collective/google-cli/internal/cmd/init" + "github.com/open-cli-collective/google-cli/internal/cmd/setcred" + "github.com/open-cli-collective/google-cli/internal/credtest" + "github.com/open-cli-collective/google-cli/internal/keychain" + "github.com/open-cli-collective/google-cli/internal/rootutil" +) + +func selectorTestRoot() *cobra.Command { + var verbose, noColor bool + root := &cobra.Command{ + Use: "grw", + PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { + return rootutil.ApplyGlobalFlags(cmd, verbose, noColor) + }, + } + rootutil.AddGlobalFlags(root, &verbose, &noColor) + root.AddCommand(initcmd.NewCommand()) + root.AddCommand(setcred.NewCmd()) + return root +} + +func TestProfileFlagInheritedByInitInBothFlagOrders(t *testing.T) { + for _, tc := range []struct { + name string + args func(string) []string + }{ + {name: "before command", args: func(path string) []string { + return []string{"--profile", "work", "init", "--credentials-file", path} + }}, + {name: "after command", args: func(path string) []string { + return []string{"init", "--profile", "work", "--credentials-file", path} + }}, + } { + t.Run(tc.name, func(t *testing.T) { + credtest.Setup(t) + t.Setenv(keychain.CredentialRefEnvVar(), "") + root := selectorTestRoot() + root.SetArgs(tc.args(filepath.Join(t.TempDir(), "missing.json"))) + if err := root.Execute(); err == nil { + t.Fatal("init should fail for the intentionally missing client file") + } + if got, set := keychain.GetCredentialRefOverride(); !set || got != "google-readwrite/work" { + t.Fatalf("selector after init path = (%q, %v), want google-readwrite/work", got, set) + } + }) + } +} + +func TestProfileFlagInheritedBySetCredentialTargetsNamedProfile(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {name: "before command", args: []string{"--profile", "work", "set-credential", "--key", "oauth_token", "--stdin"}}, + {name: "after command", args: []string{"set-credential", "--profile", "work", "--key", "oauth_token", "--stdin"}}, + } { + t.Run(tc.name, func(t *testing.T) { + credtest.Setup(t) + t.Setenv(keychain.CredentialRefEnvVar(), "") + root := selectorTestRoot() + root.SetIn(strings.NewReader(`{"access_token":"profile-token","refresh_token":"refresh"}`)) + root.SetArgs(tc.args) + if err := root.Execute(); err != nil { + t.Fatalf("set-credential: %v", err) + } + st, err := keychain.OpenRef("google-readwrite/work") + if err != nil { + t.Fatal(err) + } + tok, err := st.Token() + _ = st.Close() + if err != nil || tok.AccessToken != "profile-token" { + t.Fatalf("named profile token = %+v, err=%v", tok, err) + } + assertNoTokenAtRef(t, "google-readwrite/default") + }) + } +} + +func TestProfileAndSetCredentialRefAreMutuallyExclusive(t *testing.T) { + credtest.Setup(t) + root := selectorTestRoot() + root.SetIn(strings.NewReader(`{"access_token":"profile-token"}`)) + root.SetArgs([]string{"--profile", "work", "set-credential", "--ref", "google-readwrite/other", "--key", "oauth_token", "--stdin"}) + if err := root.Execute(); err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Fatalf("profile/local --ref conflict = %v, want mutual-exclusion error", err) + } +} + +func assertNoTokenAtRef(t *testing.T, ref string) { + t.Helper() + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatal(err) + } + defer func() { _ = st.Close() }() + if has, err := st.HasToken(); err != nil || has { + t.Fatalf("%s token presence = (%v, %v), want (false, nil)", ref, has, err) + } +} diff --git a/internal/app/grw/main_test.go b/internal/app/grw/main_test.go new file mode 100644 index 0000000..0f0a83f --- /dev/null +++ b/internal/app/grw/main_test.go @@ -0,0 +1,13 @@ +package grw + +import ( + "os" + "testing" + + "github.com/open-cli-collective/google-cli/internal/config" +) + +func TestMain(m *testing.M) { + config.Register(Identity()) + os.Exit(m.Run()) +} diff --git a/internal/cmd/init/init.go b/internal/cmd/init/init.go index 3864fdc..e2d036c 100644 --- a/internal/cmd/init/init.go +++ b/internal/cmd/init/init.go @@ -27,6 +27,7 @@ import ( "github.com/open-cli-collective/google-cli/internal/config" "github.com/open-cli-collective/google-cli/internal/identitycache" "github.com/open-cli-collective/google-cli/internal/keychain" + "github.com/open-cli-collective/google-cli/internal/rootutil" "github.com/open-cli-collective/google-cli/internal/sanitize" "github.com/open-cli-collective/google-cli/internal/view" ) @@ -70,13 +71,18 @@ for your whole org, see: ` + workspaceAdminsURL + ` You can also copy your credentials.json to the clipboard and run init — it will -read, validate, and write it to the config directory for you.`, +read, validate, and write it to the config directory for you. + +To authenticate a named profile without changing the active selection, pass the +global --profile flag before init.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - if opts.profile != "" { - if _, err := applyProfileFlag(opts.profile); err != nil { - return err - } + opts.profile = "" + // --profile is registered on the application root. Keep the value + // in initOptions only for the target announcement and post-auth + // guidance; rootutil already expanded it to the keychain ref. + if f := cmd.Flag(rootutil.ProfileFlagName); f != nil && f.Changed { + opts.profile = f.Value.String() } return runWith(cmd.Context(), defaultDeps(), opts) }, @@ -86,31 +92,10 @@ read, validate, and write it to the config directory for you.`, cmd.Flags().BoolVar(&opts.noBrowser, "no-browser", false, "Don't try to open the consent URL in a browser") cmd.Flags().BoolVar(&opts.noVerify, "no-verify", false, "Skip connectivity verification after setup") cmd.Flags().BoolVar(&opts.authCodeStdin, "auth-code-stdin", false, "Read the OAuth authorization code/redirect URL from stdin (two-phase install; implies no browser-open)") - cmd.Flags().StringVar(&opts.profile, "profile", "", "Authenticate the named profile (stored as /) instead of the active one - the way to ADD an account without touching the active profile's token") return cmd } -// applyProfileFlag routes this init run at / via the same -// per-invocation override mechanism as the global --ref flag (flag-level -// precedence; the one-time migration is suppressed automatically, exactly as -// for --ref). Returns the resolved ref. -func applyProfileFlag(profile string) (string, error) { - if v, set := keychain.GetCredentialRefOverride(); set && v != "" { - return "", fmt.Errorf("--profile and --ref are mutually exclusive (--ref %s was given)", v) - } - service, _, err := credstore.ParseRef(config.DefaultCredentialRef) - if err != nil { - return "", err - } - ref, err := credstore.FormatRef(service, profile) - if err != nil { - return "", fmt.Errorf("invalid profile name %q (allowed characters: letters, digits, '-', '_'): %w", profile, err) - } - keychain.SetCredentialRefOverride(ref, true) - return ref, nil -} - // initDeps groups every external collaborator the wizard touches. Tests // override individual fields; production uses defaultDeps(). type initDeps struct { @@ -431,7 +416,7 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { target = fmt.Sprintf("%s (%s)", ref, sanitize.Output(cachedEmail)) } if opts.profile == "" { - d.View.Printf("To add a different account instead, use '%s init --profile '.\n", config.ProductName()) + d.View.Printf("To add a different account instead, use '%s --profile init'.\n", config.ProductName()) } d.View.Println("") } @@ -570,7 +555,7 @@ func finishRun(d initDeps, opts *initOptions, targetRef string) error { d.View.Println("") d.View.Printf("Profile %s is authenticated but not active.\n", targetRef) d.View.Printf("Make it active: %s profiles use %s\n", prod, opts.profile) - d.View.Printf("Use per invocation: %s --ref %s \n", prod, targetRef) + d.View.Printf("Use per invocation: %s --profile %s \n", prod, opts.profile) return nil } diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index 903e606..871db34 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -18,7 +18,6 @@ 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" "github.com/open-cli-collective/google-cli/internal/testutil" "github.com/open-cli-collective/google-cli/internal/view" ) @@ -1043,39 +1042,6 @@ func TestRunWith_EnsureMigratedRunsFirst(t *testing.T) { // ---- target announcement, --profile, identity recording ------------------- -func TestApplyProfileFlag(t *testing.T) { - // Not Parallel: mutates the package-global credential-ref override. - t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) - - t.Run("valid name routes the run at service/name", func(t *testing.T) { - keychain.SetCredentialRefOverride("", false) - ref, err := applyProfileFlag("work") - if err != nil { - t.Fatalf("applyProfileFlag: %v", err) - } - if ref != "google-readonly/work" { - t.Errorf("ref = %q, want google-readonly/work", ref) - } - if v, set := keychain.GetCredentialRefOverride(); !set || v != "google-readonly/work" { - t.Errorf("override = (%q,%v), want (google-readonly/work,true)", v, set) - } - }) - - t.Run("invalid characters rejected", func(t *testing.T) { - keychain.SetCredentialRefOverride("", false) - if _, err := applyProfileFlag("user@example.com"); err == nil { - t.Fatal("expected error for '@' in profile name") - } - }) - - t.Run("conflict with --ref rejected", func(t *testing.T) { - keychain.SetCredentialRefOverride("google-readonly/other", true) - if _, err := applyProfileFlag("work"); err == nil || !strings.Contains(err.Error(), "mutually exclusive") { - t.Fatalf("expected mutual-exclusion error, got %v", err) - } - }) -} - // TestRunWithAnnouncesTarget pins the up-front naming: which profile this // run touches, where it was selected, and which account it currently holds — // BEFORE any prompt or write. @@ -1104,7 +1070,7 @@ func TestRunWithAnnouncesTarget(t *testing.T) { for _, want := range []string{ "Setting up profile: google-readonly/default (via config.yml credential_ref)", "Currently holds: ada@example.com", - "init --profile ", + "--profile init", "Token for google-readonly/default saved to test", } { if !strings.Contains(got, want) { @@ -1228,7 +1194,7 @@ func TestRunWithProfileFlagGuidance(t *testing.T) { "Setting up profile: google-readonly/work (via --profile flag)", "authenticated but not active", "profiles use work", - "--ref google-readonly/work", + "--profile work ", } { if !strings.Contains(got, want) { t.Errorf("output missing %q:\n%s", want, got) diff --git a/internal/cmd/profiles/profiles.go b/internal/cmd/profiles/profiles.go index d99dfef..20452bd 100644 --- a/internal/cmd/profiles/profiles.go +++ b/internal/cmd/profiles/profiles.go @@ -38,11 +38,12 @@ func NewCommand() *cobra.Command { Long: `Manage the credential profiles stored in the OS keyring. A profile holds one Google account's OAuth token. The active profile is the -credential_ref in config.yml (overridable per invocation with --ref or the -_CREDENTIAL_REF environment variable).`, +credential_ref in config.yml (overridable per invocation with --profile or +--ref, or with the _CREDENTIAL_REF environment variable).`, } cmd.AddCommand(newListCommand()) cmd.AddCommand(newUseCommand()) + cmd.AddCommand(newRenameCommand()) return cmd } @@ -52,6 +53,23 @@ credential_ref in config.yml (overridable per invocation with --ref or the // see what exists. var OpenStore = keychain.OpenNoMigrate +// OpenRefStore opens the source profile explicitly. Rename must use this +// seam rather than the active store so a global selector cannot redirect the +// positional source. +var OpenRefStore = keychain.OpenRef + +var ( + // These seams keep the failure ordering testable without touching a real + // keyring or config file. The production path still uses the concrete + // credstore-backed operations directly. + renameCopy = func(st *keychain.Store, oldProfile, newProfile string) error { + return st.CopyProfile(oldProfile, newProfile) + } + renameDelete = func(st *keychain.Store, profile string) error { return st.DeleteProfile(profile) } + renameSaveConfig = config.SaveConfig + renameIdentity = identitycache.Rename +) + // VerifyRef live-verifies one profile's token by asking the Gmail profile // for its email (gmail scope is granted by every CLI built on this library). // Var so tests can substitute. @@ -186,7 +204,7 @@ func runList(ctx context.Context, jsonOut, check bool) error { prod := config.ProductName() fmt.Println() fmt.Printf("Active: %s (via %s)\n", activeRef, keychain.DescribeRefSource(st.RefSource())) - fmt.Printf("Switch with '%s profiles use ', or per invocation with --ref.\n", prod) + fmt.Printf("Switch with '%s profiles use ', or per invocation with --profile .\n", prod) for _, r := range rows { if r.Active && !r.TokenPresent { fmt.Printf("The active profile has no stored token - run '%s init' to authenticate it.\n", prod) @@ -309,6 +327,94 @@ func runUse(arg string) error { return nil } +func newRenameCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "rename ", + Short: "Rename a credential profile", + Long: `Rename a profile in this CLI's credential namespace without +re-authenticating it. The destination must not already contain credentials; +other profiles remain unchanged. The saved active profile is updated when it +points at the old name.`, + Args: cobra.ExactArgs(2), + RunE: func(_ *cobra.Command, args []string) error { + return runRename(args[0], args[1]) + }, + } + return cmd +} + +func runRename(oldProfile, newProfile string) error { + service, _, err := credstore.ParseRef(config.DefaultCredentialRef) + if err != nil { + return fmt.Errorf("resolve CLI service: %w", err) + } + oldRef, err := credstore.FormatRef(service, oldProfile) + if err != nil { + return fmt.Errorf("invalid old profile %q: %w", oldProfile, err) + } + newRef, err := credstore.FormatRef(service, newProfile) + if err != nil { + return fmt.Errorf("invalid new profile %q: %w", newProfile, err) + } + + // Load the persisted binding before touching credentials. Runtime selector + // overrides are intentionally absent here: rename's positional old name is + // always the source, and only the saved config binding may be rewritten. + cfg, err := config.LoadConfigForRuntime() + if err != nil { + return err + } + st, err := OpenRefStore(oldRef) + if err != nil { + return err + } + defer func() { _ = st.Close() }() + + if oldProfile == newProfile { + if err := renameCopy(st, oldProfile, newProfile); err != nil { + return err + } + fmt.Printf("Profile %s is already named %s.\n", oldRef, newRef) + return nil + } + + // Copy first: SetBundle validates every key and rolls back partial writes; + // the source is retained when the copy or any later state update fails. + if err := renameCopy(st, oldProfile, newProfile); err != nil { + return err + } + + activeChanged := cfg.CredentialRef == oldRef + if activeChanged { + cfg.CredentialRef = newRef + cfg.SetCredentialRefSource(config.RefSourceConfig) + if err := renameSaveConfig(cfg); err != nil { + return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained: %w", oldRef, err) + } + } + + // Delete only after the destination and any required config update are in + // place. A partial delete leaves the destination copy, so no token is lost. + if err := renameDelete(st, oldProfile); err != nil { + return fmt.Errorf("profile copied to %s but source %s could not be removed: %w", newRef, oldRef, err) + } + + // Identity data is disposable, but preserving its verification timestamp + // makes the rename transparent to `profiles list`. + if err := renameIdentity(oldProfile, newProfile); err != nil { + fmt.Fprintf(os.Stderr, "warning: credentials renamed from %s to %s but cached identity was not moved: %v\n", oldRef, newRef, err) + } + + fmt.Printf("Renamed profile %s to %s.\n", oldRef, newRef) + if activeChanged { + fmt.Printf("Active profile is now %s.\n", newRef) + } + if env := os.Getenv(keychain.CredentialRefEnvVar()); env == oldRef { + fmt.Printf("Note: %s still points to %s; update it in this shell.\n", keychain.CredentialRefEnvVar(), oldRef) + } + return nil +} + func presence(ok bool) string { if ok { return "present" diff --git a/internal/cmd/profiles/profiles_test.go b/internal/cmd/profiles/profiles_test.go index b53ad76..f796ad2 100644 --- a/internal/cmd/profiles/profiles_test.go +++ b/internal/cmd/profiles/profiles_test.go @@ -44,6 +44,26 @@ func capture(t *testing.T, f func()) string { return <-done } +func captureStderr(t *testing.T, f func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + orig := os.Stderr + os.Stderr = w + done := make(chan string, 1) + go func() { var b bytes.Buffer; _, _ = io.Copy(&b, r); done <- b.String() }() + func() { + defer func() { + os.Stderr = orig + _ = w.Close() + }() + f() + }() + return <-done +} + // seedToken stores a token under the given profile of the test service. func seedToken(t *testing.T, profile string) { t.Helper() @@ -73,7 +93,7 @@ func TestNewCommandSurface(t *testing.T) { for _, c := range cmd.Commands() { names = append(names, c.Name()) } - for _, want := range []string{"list", "use"} { + for _, want := range []string{"list", "use", "rename"} { found := false for _, n := range names { if n == want { @@ -323,6 +343,307 @@ func TestRunUse_InvalidProfileRejected(t *testing.T) { } } +func TestRunRename_MovesTokenCacheAndImplicitActiveProfile(t *testing.T) { + credtest.Setup(t) + seedToken(t, "default") + if err := identitycache.Put("default", "default@example.com"); err != nil { + t.Fatal(err) + } + + out := capture(t, func() { + if err := runRename("default", "primary"); err != nil { + t.Errorf("runRename: %v", err) + } + }) + for _, want := range []string{ + "Renamed profile google-readonly/default to google-readonly/primary.", + "Active profile is now google-readonly/primary.", + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q:\n%s", want, out) + } + } + + cfg, err := config.LoadConfigForRuntime() + if err != nil { + t.Fatal(err) + } + if cfg.CredentialRef != "google-readonly/primary" { + t.Fatalf("credential_ref = %q, want google-readonly/primary", cfg.CredentialRef) + } + old, err := keychain.OpenRef("google-readonly/default") + if err != nil { + t.Fatal(err) + } + oldHas, err := old.HasToken() + _ = old.Close() + if err != nil || oldHas { + t.Fatalf("old token after rename = (%v, %v), want (false, nil)", oldHas, err) + } + newStore, err := keychain.OpenRef("google-readonly/primary") + if err != nil { + t.Fatal(err) + } + newTok, err := newStore.Token() + _ = newStore.Close() + if err != nil || newTok.AccessToken != "A-default" { + t.Fatalf("new token after rename = %+v, err=%v", newTok, err) + } + cached := identitycache.Load() + if _, ok := cached["default"]; ok { + t.Fatal("old cached identity remains after rename") + } + if got := cached["primary"].Email; got != "default@example.com" { + t.Fatalf("new cached identity = %q, want default@example.com", got) + } +} + +func TestRunRename_CollisionRetainsSourceAndDestination(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + seedToken(t, "new") + + err := runRenameQuiet(t, "old", "new") + if err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("rename collision = %v, want occupied-destination error", err) + } + cfg, cfgErr := config.LoadConfigForRuntime() + if cfgErr != nil { + t.Fatal(cfgErr) + } + if cfg.CredentialRef != "google-readonly/default" { + t.Fatalf("credential_ref after collision = %q, want default", cfg.CredentialRef) + } + for _, tc := range []struct { + profile string + access string + }{ + {profile: "old", access: "A-old"}, + {profile: "new", access: "A-new"}, + } { + st, openErr := keychain.OpenRef("google-readonly/" + tc.profile) + if openErr != nil { + t.Fatal(openErr) + } + tok, tokErr := st.Token() + _ = st.Close() + if tokErr != nil || tok.AccessToken != tc.access { + t.Errorf("%s token after collision = %+v, err=%v", tc.profile, tok, tokErr) + } + } +} + +func TestRunRename_CopyFailureRetainsSource(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + original := renameCopy + renameCopy = func(_ *keychain.Store, _, _ string) error { return errors.New("copy failed") } + t.Cleanup(func() { renameCopy = original }) + + err := runRenameQuiet(t, "old", "new") + if err == nil || !strings.Contains(err.Error(), "copy failed") { + t.Fatalf("copy failure = %v, want injected error", err) + } + assertToken(t, "old", "A-old") + assertNoToken(t, "new") +} + +func TestRunRename_ConfigFailureRetainsBothBundles(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + if err := config.SaveConfig(&config.Config{CredentialRef: "google-readonly/old"}); err != nil { + t.Fatal(err) + } + original := renameSaveConfig + renameSaveConfig = func(*config.Config) error { return errors.New("config unavailable") } + t.Cleanup(func() { renameSaveConfig = original }) + + err := runRenameQuiet(t, "old", "new") + if err == nil || !strings.Contains(err.Error(), "source was retained") { + t.Fatalf("config failure = %v, want source-retained error", err) + } + assertToken(t, "old", "A-old") + assertToken(t, "new", "A-old") + cfg, loadErr := config.LoadConfigForRuntime() + if loadErr != nil { + t.Fatal(loadErr) + } + if cfg.CredentialRef != "google-readonly/old" { + t.Fatalf("credential_ref after config failure = %q, want old", cfg.CredentialRef) + } +} + +func TestRunRename_DeleteFailureRetainsBothBundles(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + if err := config.SaveConfig(&config.Config{CredentialRef: "google-readonly/old"}); err != nil { + t.Fatal(err) + } + original := renameDelete + renameDelete = func(_ *keychain.Store, _ string) error { return errors.New("delete failed") } + t.Cleanup(func() { renameDelete = original }) + + err := runRenameQuiet(t, "old", "new") + if err == nil || !strings.Contains(err.Error(), "could not be removed") { + t.Fatalf("delete failure = %v, want source-removal error", err) + } + assertToken(t, "old", "A-old") + assertToken(t, "new", "A-old") + cfg, loadErr := config.LoadConfigForRuntime() + if loadErr != nil { + t.Fatal(loadErr) + } + if cfg.CredentialRef != "google-readonly/new" { + t.Fatalf("credential_ref after delete failure = %q, want new", cfg.CredentialRef) + } +} + +func TestRunRename_IgnoresInvocationSelectorForSource(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + keychain.SetCredentialRefOverride("google-readonly/other", true) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + + if err := runRenameQuiet(t, "old", "new"); err != nil { + t.Fatalf("runRename with selector override: %v", err) + } + assertToken(t, "new", "A-old") + assertNoToken(t, "other") +} + +func TestRunRename_ReplacesStaleCachedDestination(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + if err := identitycache.Put("old", "old@example.com"); err != nil { + t.Fatal(err) + } + if err := identitycache.Put("new", "stale@example.com"); err != nil { + t.Fatal(err) + } + if err := runRenameQuiet(t, "old", "new"); err != nil { + t.Fatalf("runRename: %v", err) + } + cached := identitycache.Load() + if _, ok := cached["old"]; ok { + t.Fatal("old cached identity remains after rename") + } + if got := cached["new"].Email; got != "old@example.com" { + t.Fatalf("destination cached identity = %q, want old@example.com", got) + } +} + +func TestRunRename_RemovesStaleCachedDestinationWithoutSourceCache(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + if err := identitycache.Put("new", "stale@example.com"); err != nil { + t.Fatal(err) + } + if err := runRenameQuiet(t, "old", "new"); err != nil { + t.Fatalf("runRename: %v", err) + } + if _, ok := identitycache.Load()["new"]; ok { + t.Fatal("stale destination cached identity remains") + } +} + +func TestRunRename_CacheFailureWarnsAfterCredentialSuccess(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + original := renameIdentity + renameIdentity = func(_, _ string) error { return errors.New("cache unavailable") } + t.Cleanup(func() { renameIdentity = original }) + + var runErr error + out := capture(t, func() { + stderr := captureStderr(t, func() { runErr = runRename("old", "new") }) + if !strings.Contains(stderr, "cached identity was not moved") { + t.Errorf("stderr = %q, want cache warning", stderr) + } + }) + if runErr != nil { + t.Fatalf("runRename with cache failure: %v", runErr) + } + if !strings.Contains(out, "Renamed profile google-readonly/old to google-readonly/new.") { + t.Fatalf("stdout = %q, want successful rename", out) + } + assertNoToken(t, "old") + assertToken(t, "new", "A-old") +} + +func assertToken(t *testing.T, profile, want string) { + t.Helper() + st, err := keychain.OpenRef("google-readonly/" + profile) + if err != nil { + t.Fatal(err) + } + defer func() { _ = st.Close() }() + tok, err := st.Token() + if err != nil || tok.AccessToken != want { + t.Fatalf("%s token = %+v, err=%v; want access token %q", profile, tok, err, want) + } +} + +func assertNoToken(t *testing.T, profile string) { + t.Helper() + st, err := keychain.OpenRef("google-readonly/" + profile) + if err != nil { + t.Fatal(err) + } + defer func() { _ = st.Close() }() + if has, err := st.HasToken(); err != nil || has { + t.Fatalf("%s token presence = (%v, %v), want (false, nil)", profile, has, err) + } +} + +func TestRunRenameWarnsWhenEnvironmentStillNamesOldRef(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + t.Setenv(keychain.CredentialRefEnvVar(), "google-readonly/old") + + out := capture(t, func() { + if err := runRename("old", "new"); err != nil { + t.Errorf("runRename: %v", err) + } + }) + for _, want := range []string{keychain.CredentialRefEnvVar(), "update it in this shell"} { + if !strings.Contains(out, want) { + t.Errorf("output missing %q:\n%s", want, out) + } + } +} + +func TestRunRenameSameProfileIsNoOp(t *testing.T) { + credtest.Setup(t) + seedToken(t, "work") + + out := capture(t, func() { + if err := runRename("work", "work"); err != nil { + t.Errorf("runRename: %v", err) + } + }) + if !strings.Contains(out, "already named") { + t.Fatalf("same-profile output = %q, want no-op message", out) + } + st, err := keychain.OpenRef("google-readonly/work") + if err != nil { + t.Fatal(err) + } + has, err := st.HasToken() + _ = st.Close() + if err != nil || !has { + t.Fatalf("token after same-profile rename = (%v, %v), want (true, nil)", has, err) + } +} + +func TestRunRename_MissingSourceRejected(t *testing.T) { + credtest.Setup(t) + err := runRenameQuiet(t, "missing", "new") + if err == nil || !errors.Is(err, keychain.ErrProfileNotFound) { + t.Fatalf("missing source error = %v, want ErrProfileNotFound", err) + } + assertNoToken(t, "new") +} + // runUseQuiet runs runUse with stdout swallowed (these cases assert on error // or config state, not output). func runUseQuiet(t *testing.T, arg string) error { @@ -331,3 +652,10 @@ func runUseQuiet(t *testing.T, arg string) error { capture(t, func() { err = runUse(arg) }) return err } + +func runRenameQuiet(t *testing.T, oldProfile, newProfile string) error { + t.Helper() + var err error + capture(t, func() { err = runRename(oldProfile, newProfile) }) + return err +} diff --git a/internal/cmd/setcred/setcred.go b/internal/cmd/setcred/setcred.go index a6a5f31..3de1aaa 100644 --- a/internal/cmd/setcred/setcred.go +++ b/internal/cmd/setcred/setcred.go @@ -83,23 +83,31 @@ func run(opts *options) error { return fmt.Errorf("token has neither an access nor a refresh token") } - // §1.8: when targeting the default ref, run the one-time legacy migration - // first (shared keychain.EnsureMigrated, same guarantee as init). + // §1.8: when targeting the configured/default ref, run the one-time legacy + // migration first (shared keychain.EnsureMigrated, same guarantee as init). // Otherwise a pre-existing legacy token.json + this fresh keyring write // would collide on the next real command's Open() with a §1.8 conflict. A // genuine conflict here aborts loudly (the user must resolve it, not - // silently overwrite via this scriptable path). An explicit --ref never - // migrates — the one-time migration only ever targets the canonical - // configured ref (see keychain.OpenRef). + // silently overwrite via this scriptable path). An explicit selector (local + // --ref or global --profile) never migrates — the one-time migration only + // ever targets the canonical configured ref (see keychain.OpenRef). + targetRef := opts.ref + if targetRef == "" { + if ref, set := keychain.GetCredentialRefOverride(); set && ref != "" { + targetRef = ref + } else if ref := os.Getenv(keychain.CredentialRefEnvVar()); ref != "" { + targetRef = ref + } + } migrated := false - if opts.ref == "" { + if targetRef == "" { if merr := keychain.EnsureMigrated(); merr != nil { return merr } migrated = true } - st, err := keychain.OpenRef(opts.ref) // ingress: runMigration=false + st, err := keychain.OpenRef(targetRef) // ingress: runMigration=false if err != nil { if migrated { // The legacy original may already have been consumed by the diff --git a/internal/cmd/setcred/setcred_test.go b/internal/cmd/setcred/setcred_test.go index 16a9801..d0c18a1 100644 --- a/internal/cmd/setcred/setcred_test.go +++ b/internal/cmd/setcred/setcred_test.go @@ -60,6 +60,25 @@ func TestSetCredentialFromEnvSuccess(t *testing.T) { } } +func TestSetCredentialEmptySelectorFallsThroughToEnvironment(t *testing.T) { + credtest.Setup(t) + t.Setenv(keychain.CredentialRefEnvVar(), "google-readonly/env") + keychain.SetCredentialRefOverride("", true) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + + if err := run(&options{key: keychain.KeyOAuthToken, stdin: true, in: strings.NewReader(tokenJSON)}); err != nil { + t.Fatalf("set-credential with empty selector: %v", err) + } + st, err := keychain.OpenRef("google-readonly/env") + if err != nil { + t.Fatal(err) + } + defer func() { _ = st.Close() }() + if tok, err := st.Token(); err != nil || tok.AccessToken != "SECRET-ACCESS" { + t.Fatalf("environment target token = %+v, err=%v", tok, err) + } +} + func TestSetCredentialRejectsNonToken(t *testing.T) { credtest.Setup(t) err := run(&options{key: keychain.KeyOAuthToken, stdin: true, in: strings.NewReader(`{"not":"a token"}`)}) diff --git a/internal/identitycache/identitycache.go b/internal/identitycache/identitycache.go index 3798665..9c5c5ea 100644 --- a/internal/identitycache/identitycache.go +++ b/internal/identitycache/identitycache.go @@ -80,3 +80,34 @@ func Put(profile, email string) error { } return clicache.WriteResource(loc, resourceName, ttl, m) } + +// Rename moves a cached identity to another profile while preserving its +// verification time. Credentials are authoritative: an existing destination +// identity is replaced, and a missing source removes any stale destination +// identity. A cache miss with no destination is a successful no-op. +func Rename(oldProfile, newProfile string) error { + if oldProfile == "" || newProfile == "" { + return fmt.Errorf("identitycache: old and new profiles are required") + } + if oldProfile == newProfile { + return nil + } + + m := Load() + entry, ok := m[oldProfile] + if !ok { + if _, destination := m[newProfile]; !destination { + return nil + } + delete(m, newProfile) + } else { + m[newProfile] = entry + } + delete(m, oldProfile) + + loc, err := locator() + if err != nil { + return err + } + return clicache.WriteResource(loc, resourceName, ttl, m) +} diff --git a/internal/identitycache/identitycache_test.go b/internal/identitycache/identitycache_test.go index 9d36d60..2d95d89 100644 --- a/internal/identitycache/identitycache_test.go +++ b/internal/identitycache/identitycache_test.go @@ -64,6 +64,57 @@ func TestPutRejectsEmpty(t *testing.T) { } } +func TestRenamePreservesIdentityAndVerificationTime(t *testing.T) { + credtest.Setup(t) + if err := Put("old", "user@example.com"); err != nil { + t.Fatal(err) + } + want := Load()["old"] + if err := Rename("old", "new"); err != nil { + t.Fatal(err) + } + m := Load() + if _, ok := m["old"]; ok { + t.Fatal("old identity remains after rename") + } + if got := m["new"]; got != want { + t.Errorf("renamed identity = %+v, want %+v", got, want) + } +} + +func TestRenameReplacesOccupiedDestination(t *testing.T) { + credtest.Setup(t) + if err := Put("old", "old@example.com"); err != nil { + t.Fatal(err) + } + if err := Put("new", "new@example.com"); err != nil { + t.Fatal(err) + } + if err := Rename("old", "new"); err != nil { + t.Fatal(err) + } + m := Load() + if _, ok := m["old"]; ok { + t.Errorf("old identity remains after replacement: %+v", m) + } + if m["new"].Email != "old@example.com" { + t.Errorf("destination identity = %+v, want source identity", m["new"]) + } +} + +func TestRenameRemovesStaleDestinationWhenSourceMissing(t *testing.T) { + credtest.Setup(t) + if err := Put("new", "stale@example.com"); err != nil { + t.Fatal(err) + } + if err := Rename("old", "new"); err != nil { + t.Fatal(err) + } + if _, ok := Load()["new"]; ok { + t.Fatal("stale destination identity remains after source-missing rename") + } +} + func TestLoadToleratesCorruptFile(t *testing.T) { credtest.Setup(t) dir, err := config.GetCacheDir() diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 3eacc80..9b1cd24 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -38,6 +38,11 @@ var allowedKeys = []string{KeyOAuthToken} // wrapper of credstore.ErrNotFound). Name retained for existing callers. var ErrTokenNotFound = errors.New("no token found in secure storage") +// ErrProfileNotFound indicates that a profile has no stored credential +// bundle. It is used by profile management so a typo cannot silently create +// an empty destination. +var ErrProfileNotFound = errors.New("profile has no stored credentials") + // Store is an open handle to gro's credential bundle. Construct with one of // the Open* functions, always Close. It carries the resolved ref so callers // can report it in `config show` / errors without re-deriving it (the ref is @@ -329,6 +334,57 @@ func (s *Store) HasTokenFor(profile string) (bool, error) { return ok, nil } +// CopyProfile copies every stored key in oldProfile to newProfile. The +// destination must be empty; SetBundle validates all keys before writing and +// rolls back a partial write where the backend supports it. The source is +// untouched on every copy failure. credstore has no cross-process +// compare-and-swap, so concurrent writers are outside this operation's +// transaction boundary. +func (s *Store) CopyProfile(oldProfile, newProfile string) error { + oldKeys, err := s.cs.ListBundle(oldProfile) + if err != nil { + return fmt.Errorf("list source profile %q: %w", oldProfile, err) + } + if len(oldKeys) == 0 { + return fmt.Errorf("%w: %s/%s", ErrProfileNotFound, s.service, oldProfile) + } + if oldProfile == newProfile { + return nil + } + + newKeys, err := s.cs.ListBundle(newProfile) + if err != nil { + return fmt.Errorf("list destination profile %q: %w", newProfile, err) + } + if len(newKeys) > 0 { + return fmt.Errorf("destination profile %s/%s already exists", s.service, newProfile) + } + + bundle := make(map[string]string, len(oldKeys)) + for _, key := range oldKeys { + value, err := s.cs.Get(oldProfile, key) + if err != nil { + return fmt.Errorf("read %s/%s/%s: %w", s.service, oldProfile, key, err) + } + bundle[key] = value + } + if _, err := s.cs.SetBundle(newProfile, bundle); err != nil { + return fmt.Errorf("copy profile %s/%s to %s/%s: %w", s.service, oldProfile, s.service, newProfile, err) + } + return nil +} + +// DeleteProfile removes every key stored under profile. It keeps the source +// bundle semantics in one place so callers can report partial deletion +// without ever claiming a complete rename. A concurrent writer can race this +// operation because credstore exposes no cross-process lock or CAS. +func (s *Store) DeleteProfile(profile string) error { + if _, err := s.cs.DeleteBundle(profile); err != nil { + return fmt.Errorf("delete profile %s/%s: %w", s.service, profile, err) + } + return nil +} + // EnsureMigrated runs (and resolves) the one-time §1.8 legacy migration up // front via the full Open() path, then closes. A legacy-vs-keyring conflict // surfaces as a hard error. Shared by `gro init` and `gro set-credential` so diff --git a/internal/keychain/profiles_test.go b/internal/keychain/profiles_test.go index 1d086d9..a217e38 100644 --- a/internal/keychain/profiles_test.go +++ b/internal/keychain/profiles_test.go @@ -1,8 +1,11 @@ package keychain import ( + "errors" + "strings" "testing" + "github.com/open-cli-collective/cli-common/credstore" "golang.org/x/oauth2" "github.com/open-cli-collective/google-cli/internal/config" @@ -54,3 +57,107 @@ func TestListProfilesAndHasTokenFor(t *testing.T) { t.Fatalf("HasTokenFor(absent) = (%v, %v), want (false, nil)", has, herr) } } + +func TestProfileCopyDeleteMovesBundleWithoutReauth(t *testing.T) { + credtest.Setup(t) + st, err := openWith(testCfg(), false, false) + if err != nil { + t.Fatalf("open: %v", err) + } + defer func() { _ = st.Close() }() + if err := st.SetToken(&oauth2.Token{AccessToken: "A", RefreshToken: "R"}); err != nil { + t.Fatal(err) + } + + if err := st.CopyProfile("default", "work"); err != nil { + t.Fatalf("CopyProfile: %v", err) + } + if err := st.DeleteProfile("default"); err != nil { + t.Fatalf("DeleteProfile: %v", err) + } + if _, err := st.Token(); !errors.Is(err, ErrTokenNotFound) { + t.Fatalf("source token after rename = %v, want not found", err) + } + work, err := openWith(&config.Config{CredentialRef: "google-readonly/work"}, false, false) + if err != nil { + t.Fatalf("open destination: %v", err) + } + defer func() { _ = work.Close() }() + tok, err := work.Token() + if err != nil || tok.AccessToken != "A" || tok.RefreshToken != "R" { + t.Fatalf("destination token = %+v, err=%v", tok, err) + } +} + +func TestCopyProfileCollisionRetainsBothBundles(t *testing.T) { + credtest.Setup(t) + st, err := openWith(testCfg(), false, false) + if err != nil { + t.Fatalf("open: %v", err) + } + defer func() { _ = st.Close() }() + if err := st.SetToken(&oauth2.Token{AccessToken: "OLD", RefreshToken: "R"}); err != nil { + t.Fatal(err) + } + work, err := openWith(&config.Config{CredentialRef: "google-readonly/work"}, false, false) + if err != nil { + t.Fatalf("open destination: %v", err) + } + if err := work.SetToken(&oauth2.Token{AccessToken: "NEW", RefreshToken: "R"}); err != nil { + t.Fatal(err) + } + _ = work.Close() + + err = st.CopyProfile("default", "work") + if err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("CopyProfile collision = %v, want occupied-destination error", err) + } + old, err := st.Token() + if err != nil || old.AccessToken != "OLD" { + t.Fatalf("source token after collision = %+v, err=%v", old, err) + } + work, err = openWith(&config.Config{CredentialRef: "google-readonly/work"}, false, false) + if err != nil { + t.Fatalf("reopen destination: %v", err) + } + defer func() { _ = work.Close() }() + newTok, err := work.Token() + if err != nil || newTok.AccessToken != "NEW" { + t.Fatalf("destination token after collision = %+v, err=%v", newTok, err) + } +} + +func TestProfileCopyDeleteDoesNotCrossServiceNamespace(t *testing.T) { + credtest.Setup(t) + readonly, err := openWith(testCfg(), false, false) + if err != nil { + t.Fatalf("open readonly: %v", err) + } + defer func() { _ = readonly.Close() }() + if err := readonly.SetToken(&oauth2.Token{AccessToken: "READONLY", RefreshToken: "R"}); err != nil { + t.Fatal(err) + } + + service := "google-readwrite" + t.Setenv(credstore.BackendEnvVar(service), "file") + t.Setenv(strings.TrimSuffix(credstore.BackendEnvVar(service), "_KEYRING_BACKEND")+"_KEYRING_PASSPHRASE", "test-passphrase") + readwrite, err := openWith(&config.Config{CredentialRef: service + "/default"}, false, false) + if err != nil { + t.Fatalf("open readwrite: %v", err) + } + defer func() { _ = readwrite.Close() }() + if err := readwrite.SetToken(&oauth2.Token{AccessToken: "READWRITE", RefreshToken: "R"}); err != nil { + t.Fatal(err) + } + + if err := readonly.CopyProfile("default", "renamed"); err != nil { + t.Fatalf("copy readonly: %v", err) + } + if err := readonly.DeleteProfile("default"); err != nil { + t.Fatalf("delete readonly: %v", err) + } + got, err := readwrite.Token() + if err != nil || got.AccessToken != "READWRITE" { + t.Fatalf("readwrite token after readonly rename = %+v, err=%v", got, err) + } +} diff --git a/internal/rootutil/rootutil.go b/internal/rootutil/rootutil.go index 2bd629f..f9fc97e 100644 --- a/internal/rootutil/rootutil.go +++ b/internal/rootutil/rootutil.go @@ -18,6 +18,7 @@ import ( cccredstore "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" "github.com/open-cli-collective/google-cli/internal/log" "github.com/open-cli-collective/google-cli/internal/migrationsink" @@ -28,6 +29,10 @@ import ( // that shadows this persistent one for that command only). const CredentialRefFlagName = "ref" +// ProfileFlagName is the global bare-profile selector. It expands to the +// registered CLI's service/profile credential ref for one invocation. +const ProfileFlagName = "profile" + // AddGlobalFlags registers the standard persistent flags on cmd, binding // verbose and noColor to the given pointers. The --ref help shows the env var // by its _ pattern rather than the resolved name because flags are @@ -41,6 +46,9 @@ func AddGlobalFlags(cmd *cobra.Command, verbose, noColor *bool) { "can target different accounts without racing on config.yml "+ "(precedence: --%s flag > _CREDENTIAL_REF env > config credential_ref)", CredentialRefFlagName)) + cmd.PersistentFlags().String(ProfileFlagName, "", fmt.Sprintf( + "Profile name for this invocation (shorthand for --%s /)", + CredentialRefFlagName)) } // ApplyGlobalFlags runs the shared PersistentPreRunE logic: verbosity, color, @@ -77,24 +85,62 @@ func WireBackendSelection(cmd *cobra.Command) error { return nil } -// WireCredentialRefSelection records the user-supplied --ref flag for the next -// keychain.Open* call and validates its / shape up front so a -// bad value fails with a clear "--ref" error before any keyring work. The -// resolved precedence (--ref flag > _CREDENTIAL_REF env > config -// credential_ref) is applied at keychain.open; this hook only records the flag. +// WireCredentialRefSelection records the user-supplied --ref/--profile +// selector for the next keychain.Open* call. --profile is expanded using the +// registered CLI's service, while --ref keeps accepting a full ref. Both are +// explicit selectors and cannot be supplied together. An empty --profile is +// rejected; an empty --ref retains its historical fall-through to env/config. +// The resolved precedence (explicit selector > env > config > built-in +// default) is applied at keychain.open. func WireCredentialRefSelection(cmd *cobra.Command) error { - f := cmd.Flag(CredentialRefFlagName) - if f == nil { - return nil + refFlag := cmd.Flag(CredentialRefFlagName) + profileFlag := cmd.Flag(ProfileFlagName) + refSet := refFlag != nil && refFlag.Changed + // set-credential intentionally shadows the root --ref with its local + // write-target flag. Still inspect the root flag for the mutual-exclusion + // check so `--ref ... --profile ... set-credential` cannot slip through + // based on flag order; when no --profile is present, preserve the local + // flag's historical precedence and ignore a shadowed root value. + rootRefSet := false + if root := cmd.Root(); root != nil { + if rootRef := root.PersistentFlags().Lookup(CredentialRefFlagName); rootRef != nil && rootRef != refFlag { + rootRefSet = rootRef.Changed + } + } + profileSet := profileFlag != nil && profileFlag.Changed + if (refSet || rootRefSet) && profileSet { + return fmt.Errorf("--%s and --%s are mutually exclusive; choose one", ProfileFlagName, CredentialRefFlagName) } - value := f.Value.String() - changed := f.Changed - if changed && value != "" { - if _, _, err := cccredstore.ParseRef(value); err != nil { - return fmt.Errorf("--%s: %w", CredentialRefFlagName, err) + + switch { + case profileSet: + profile := profileFlag.Value.String() + if profile == "" { + return fmt.Errorf("--%s requires a non-empty profile name", ProfileFlagName) + } + service, _, err := cccredstore.ParseRef(config.DefaultCredentialRef) + if err != nil { + return fmt.Errorf("resolving CLI service for --%s: %w", ProfileFlagName, err) + } + ref, err := cccredstore.FormatRef(service, profile) + if err != nil { + return fmt.Errorf("--%s: invalid profile name %q: %w", ProfileFlagName, profile, err) + } + keychain.SetCredentialRefOverride(ref, true) + case refSet: + value := refFlag.Value.String() + if value != "" { + if _, _, err := cccredstore.ParseRef(value); err != nil { + return fmt.Errorf("--%s: %w", CredentialRefFlagName, err) + } } + // Preserve the changed/empty distinction for callers that inspect the + // override, while keychain.effectiveRef intentionally falls through + // when the value is empty. + keychain.SetCredentialRefOverride(value, true) + default: + keychain.SetCredentialRefOverride("", false) } - keychain.SetCredentialRefOverride(value, changed) return nil } diff --git a/internal/rootutil/rootutil_test.go b/internal/rootutil/rootutil_test.go new file mode 100644 index 0000000..082ae7a --- /dev/null +++ b/internal/rootutil/rootutil_test.go @@ -0,0 +1,114 @@ +package rootutil + +import ( + "os" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/open-cli-collective/google-cli/internal/config" + "github.com/open-cli-collective/google-cli/internal/keychain" +) + +func TestMain(m *testing.M) { + config.RegisterForTest() + os.Exit(m.Run()) +} + +func selectorRoot() *cobra.Command { + var verbose, noColor bool + root := &cobra.Command{ + Use: "gro", + PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { + return WireCredentialRefSelection(cmd) + }, + } + AddGlobalFlags(root, &verbose, &noColor) + root.AddCommand(&cobra.Command{Use: "probe", Run: func(*cobra.Command, []string) {}}) + return root +} + +func TestWireCredentialRefSelection_ProfileExpandsToRegisteredService(t *testing.T) { + keychain.SetCredentialRefOverride("", false) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + root := selectorRoot() + root.SetArgs([]string{"--profile", "work", "probe"}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + if got, set := keychain.GetCredentialRefOverride(); !set || got != "google-readonly/work" { + t.Fatalf("override = (%q, %v), want (google-readonly/work, true)", got, set) + } +} + +func TestWireCredentialRefSelection_ProfileBeatsEnvironment(t *testing.T) { + keychain.SetCredentialRefOverride("", false) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + t.Setenv(keychain.CredentialRefEnvVar(), "google-readonly/env") + root := selectorRoot() + root.SetArgs([]string{"--profile", "work", "probe"}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + got, set := keychain.GetCredentialRefOverride() + if !set || got != "google-readonly/work" { + t.Fatalf("override = (%q, %v), want explicit profile to win", got, set) + } +} + +func TestWireCredentialRefSelection_RejectsEmptyAndBothSelectors(t *testing.T) { + for _, tc := range []struct { + name string + args []string + want string + }{ + {name: "empty profile", args: []string{"--profile=", "probe"}, want: "--profile"}, + {name: "empty ref preserves fall-through", args: []string{"--ref=", "probe"}}, + {name: "both", args: []string{"--profile", "work", "--ref", "google-readonly/other", "probe"}, want: "mutually exclusive"}, + } { + t.Run(tc.name, func(t *testing.T) { + keychain.SetCredentialRefOverride("", false) + root := selectorRoot() + root.SetArgs(tc.args) + err := root.Execute() + if tc.want == "" { + if err != nil { + t.Fatalf("empty --ref should preserve legacy fall-through: %v", err) + } + if value, set := keychain.GetCredentialRefOverride(); !set || value != "" { + t.Fatalf("override = (%q, %v), want explicit empty ref", value, set) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %v, want substring %q", err, tc.want) + } + }) + } +} + +func TestWireCredentialRefSelection_NoSelectorClearsPreviousOverride(t *testing.T) { + keychain.SetCredentialRefOverride("google-readonly/old", true) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + root := selectorRoot() + root.SetArgs([]string{"probe"}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + if got, set := keychain.GetCredentialRefOverride(); set || got != "" { + t.Fatalf("override = (%q, %v), want cleared", got, set) + } +} + +func TestWireCredentialRefSelection_SeesShadowedRootRefForConflict(t *testing.T) { + keychain.SetCredentialRefOverride("", false) + root := selectorRoot() + shadow := &cobra.Command{Use: "shadow", Run: func(*cobra.Command, []string) {}} + shadow.Flags().String("ref", "", "local write target") + root.AddCommand(shadow) + root.SetArgs([]string{"--ref", "google-readonly/root", "--profile", "work", "shadow"}) + if err := root.Execute(); err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Fatalf("shadowed --ref plus --profile = %v, want mutual-exclusion error", err) + } +} From 626c32eb2d5bbb1aa24b4108917f701a648f6db8 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 15 Sep 2026 16:10:47 -0400 Subject: [PATCH 2/5] fix: complete profile review coverage --- README.md | 14 ++++++-- internal/cmd/init/init_test.go | 17 ++++++++++ internal/cmd/profiles/profiles_test.go | 44 ++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index b6a1715..ca04052 100644 --- a/README.md +++ b/README.md @@ -82,9 +82,17 @@ gro --ref google-readonly/work mail list The selector precedence is explicit flag (`--profile` or `--ref`), credential reference environment variable, saved `credential_ref`, then the built-in -`default` profile. `--profile` and `--ref` cannot be used together. To add an -account without changing the active profile, run `gro --profile work init` (or -the equivalent `grw` command). Inspect and manage profiles with: +`default` profile. For environment selection, use +`GOOGLE_READONLY_CREDENTIAL_REF` with `gro` or +`GOOGLE_READWRITE_CREDENTIAL_REF` with `grw`, for example: + +```bash +GOOGLE_READONLY_CREDENTIAL_REF=google-readonly/work gro mail list +``` + +`--profile` and `--ref` cannot be used together. To add an account without +changing the active profile, run `gro --profile work init` (or the equivalent +`grw` command). Inspect and manage profiles with: ```bash gro profiles list diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index 871db34..486e269 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -1175,6 +1175,14 @@ func TestRunWithProfileFlagGuidance(t *testing.T) { d := baseDeps(t, fs) out := &bytes.Buffer{} d.View = view.NewWithWriters(out, out) + clientPath := filepath.Join(t.TempDir(), "client.json") + saved := &config.Config{ + CredentialRef: "google-readonly/default", + OAuthClientPath: clientPath, + Keyring: config.KeyringConfig{Backend: "file"}, + } + d.LoadConfig = func() (*config.Config, error) { return saved, nil } + d.SaveConfig = func(c *config.Config) error { *saved = *c; return nil } d.DescribeTarget = func() (string, string, string) { return "google-readonly/work", "--ref flag", "" } @@ -1200,4 +1208,13 @@ func TestRunWithProfileFlagGuidance(t *testing.T) { t.Errorf("output missing %q:\n%s", want, got) } } + if saved.CredentialRef != "google-readonly/default" { + t.Errorf("saved credential_ref after named init = %q, want google-readonly/default", saved.CredentialRef) + } + if saved.OAuthClientPath != clientPath { + t.Errorf("saved oauth_client_path after named init = %q, want unchanged path", saved.OAuthClientPath) + } + if saved.Keyring.Backend != "file" { + t.Errorf("saved keyring backend after named init = %q, want file", saved.Keyring.Backend) + } } diff --git a/internal/cmd/profiles/profiles_test.go b/internal/cmd/profiles/profiles_test.go index f796ad2..b253797 100644 --- a/internal/cmd/profiles/profiles_test.go +++ b/internal/cmd/profiles/profiles_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "os" + "path/filepath" "strings" "testing" @@ -398,6 +399,49 @@ func TestRunRename_MovesTokenCacheAndImplicitActiveProfile(t *testing.T) { } } +func TestRunRename_NonActivePreservesSavedConfig(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + clientPath := filepath.Join(t.TempDir(), "client.json") + original := &config.Config{ + CredentialRef: "google-readonly/current", + OAuthClientPath: clientPath, + GrantedScopes: []string{"scope:mail", "scope:profile"}, + Keyring: config.KeyringConfig{Backend: "file"}, + } + if err := config.SaveConfig(original); err != nil { + t.Fatal(err) + } + + if err := runRenameQuiet(t, "old", "new"); err != nil { + t.Fatalf("runRename: %v", err) + } + + got, err := config.LoadConfigForRuntime() + if err != nil { + t.Fatal(err) + } + if got.CredentialRef != original.CredentialRef { + t.Errorf("credential_ref after non-active rename = %q, want %q", got.CredentialRef, original.CredentialRef) + } + if got.OAuthClientPath != original.OAuthClientPath { + t.Errorf("oauth_client_path after non-active rename = %q, want %q", got.OAuthClientPath, original.OAuthClientPath) + } + if len(got.GrantedScopes) != len(original.GrantedScopes) { + t.Fatalf("granted_scopes after non-active rename = %v, want %v", got.GrantedScopes, original.GrantedScopes) + } + for i := range original.GrantedScopes { + if got.GrantedScopes[i] != original.GrantedScopes[i] { + t.Errorf("granted_scopes[%d] after non-active rename = %q, want %q", i, got.GrantedScopes[i], original.GrantedScopes[i]) + } + } + if got.Keyring.Backend != original.Keyring.Backend { + t.Errorf("keyring backend after non-active rename = %q, want %q", got.Keyring.Backend, original.Keyring.Backend) + } + assertNoToken(t, "old") + assertToken(t, "new", "A-old") +} + func TestRunRename_CollisionRetainsSourceAndDestination(t *testing.T) { credtest.Setup(t) seedToken(t, "old") From 1c31e089adaad2fa24774d3977bc7764aeac34ad Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 15 Sep 2026 16:15:40 -0400 Subject: [PATCH 3/5] fix: make profile rename retries safe --- README.md | 3 +- internal/cmd/profiles/profiles.go | 8 +++- internal/cmd/profiles/profiles_test.go | 66 +++++++++++++++++++++++++- 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ca04052..eabaa06 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,8 @@ gro profiles rename old-name new-name Renaming moves the stored credentials without re-authentication, updates the saved active profile when necessary, and refuses a destination that already -has credentials. +has credentials. If saving the active-profile update fails, the copied +destination is removed while the source remains so the command can be retried. ## Documentation diff --git a/internal/cmd/profiles/profiles.go b/internal/cmd/profiles/profiles.go index 20452bd..34a9d76 100644 --- a/internal/cmd/profiles/profiles.go +++ b/internal/cmd/profiles/profiles.go @@ -389,7 +389,13 @@ func runRename(oldProfile, newProfile string) error { cfg.CredentialRef = newRef cfg.SetCredentialRefSource(config.RefSourceConfig) if err := renameSaveConfig(cfg); err != nil { - return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained: %w", oldRef, err) + // The source is still intact, so remove the copy before returning. + // That makes a transient config failure retryable while preserving + // the token if rollback itself cannot complete. + if rollbackErr := renameDelete(st, newProfile); rollbackErr != nil { + return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained and copied destination may remain: %w (rollback failed: %v)", oldRef, err, rollbackErr) + } + return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained and copied destination was removed: %w", oldRef, err) } } diff --git a/internal/cmd/profiles/profiles_test.go b/internal/cmd/profiles/profiles_test.go index b253797..7bcf025 100644 --- a/internal/cmd/profiles/profiles_test.go +++ b/internal/cmd/profiles/profiles_test.go @@ -492,7 +492,7 @@ func TestRunRename_CopyFailureRetainsSource(t *testing.T) { assertNoToken(t, "new") } -func TestRunRename_ConfigFailureRetainsBothBundles(t *testing.T) { +func TestRunRename_ConfigFailureRollsBackCopy(t *testing.T) { credtest.Setup(t) seedToken(t, "old") if err := config.SaveConfig(&config.Config{CredentialRef: "google-readonly/old"}); err != nil { @@ -507,7 +507,7 @@ func TestRunRename_ConfigFailureRetainsBothBundles(t *testing.T) { t.Fatalf("config failure = %v, want source-retained error", err) } assertToken(t, "old", "A-old") - assertToken(t, "new", "A-old") + assertNoToken(t, "new") cfg, loadErr := config.LoadConfigForRuntime() if loadErr != nil { t.Fatal(loadErr) @@ -517,6 +517,68 @@ func TestRunRename_ConfigFailureRetainsBothBundles(t *testing.T) { } } +func TestRunRename_ConfigFailureRollbackFailureRetainsBothBundles(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + if err := config.SaveConfig(&config.Config{CredentialRef: "google-readonly/old"}); err != nil { + t.Fatal(err) + } + originalSave := renameSaveConfig + renameSaveConfig = func(*config.Config) error { return errors.New("config unavailable") } + originalDelete := renameDelete + renameDelete = func(st *keychain.Store, profile string) error { + if profile == "new" { + return errors.New("rollback unavailable") + } + return originalDelete(st, profile) + } + t.Cleanup(func() { + renameSaveConfig = originalSave + renameDelete = originalDelete + }) + + err := runRenameQuiet(t, "old", "new") + if err == nil || !strings.Contains(err.Error(), "rollback failed") { + t.Fatalf("config and rollback failure = %v, want rollback detail", err) + } + assertToken(t, "old", "A-old") + assertToken(t, "new", "A-old") +} + +func TestRunRename_RetryAfterTransientConfigFailure(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + if err := config.SaveConfig(&config.Config{CredentialRef: "google-readonly/old"}); err != nil { + t.Fatal(err) + } + original := renameSaveConfig + attempts := 0 + renameSaveConfig = func(cfg *config.Config) error { + attempts++ + if attempts == 1 { + return errors.New("transient config failure") + } + return original(cfg) + } + t.Cleanup(func() { renameSaveConfig = original }) + + if err := runRenameQuiet(t, "old", "new"); err == nil { + t.Fatal("first rename should fail while saving config") + } + if err := runRenameQuiet(t, "old", "new"); err != nil { + t.Fatalf("retry rename: %v", err) + } + assertNoToken(t, "old") + assertToken(t, "new", "A-old") + cfg, err := config.LoadConfigForRuntime() + if err != nil { + t.Fatal(err) + } + if cfg.CredentialRef != "google-readonly/new" { + t.Fatalf("credential_ref after retry = %q, want google-readonly/new", cfg.CredentialRef) + } +} + func TestRunRename_DeleteFailureRetainsBothBundles(t *testing.T) { credtest.Setup(t) seedToken(t, "old") From 6063e5467d4a13ae602941aa7878f8f05e79b18d Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 15 Sep 2026 16:16:28 -0400 Subject: [PATCH 4/5] fix: wrap rollback errors --- internal/cmd/profiles/profiles.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cmd/profiles/profiles.go b/internal/cmd/profiles/profiles.go index 34a9d76..3c01d9b 100644 --- a/internal/cmd/profiles/profiles.go +++ b/internal/cmd/profiles/profiles.go @@ -393,7 +393,7 @@ func runRename(oldProfile, newProfile string) error { // That makes a transient config failure retryable while preserving // the token if rollback itself cannot complete. if rollbackErr := renameDelete(st, newProfile); rollbackErr != nil { - return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained and copied destination may remain: %w (rollback failed: %v)", oldRef, err, rollbackErr) + return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained and copied destination may remain: %w (rollback failed: %w)", oldRef, err, rollbackErr) } return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained and copied destination was removed: %w", oldRef, err) } From 12d4c8394b6275b9b2bcfe930d9c9eed868d137f Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 15 Sep 2026 16:18:52 -0400 Subject: [PATCH 5/5] docs: clarify profile rename rollback --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index eabaa06..0e0e955 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,9 @@ gro profiles rename old-name new-name Renaming moves the stored credentials without re-authentication, updates the saved active profile when necessary, and refuses a destination that already has credentials. If saving the active-profile update fails, the copied -destination is removed while the source remains so the command can be retried. +destination is removed when rollback succeeds while the source remains, so the +command can be retried. If rollback also fails, the command reports that the +destination may remain. ## Documentation