From 75da336c4cad49817dfbf8d4002c0d3ad99544aa Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 15 Sep 2026 10:43:04 -0400 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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/7] 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 From 037d91223fdcc0df69a0d62ef42b4eb4d3303e42 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 20 Sep 2026 07:21:08 -0400 Subject: [PATCH 6/7] feat: support profile-scoped OAuth clients --- README.md | 2 +- WORKSPACE_ADMINS.md | 71 ++++--- internal/auth/auth.go | 25 ++- internal/auth/auth_test.go | 157 +++++++++++++++ internal/cmd/config/behavior_test.go | 59 ++++++ internal/cmd/config/config.go | 5 +- internal/cmd/init/init.go | 209 +++++++++++++++----- internal/cmd/init/init_test.go | 253 ++++++++++++++++++++----- internal/cmd/init/sibling_test.go | 6 +- internal/cmd/me/me.go | 15 +- internal/cmd/me/me_test.go | 36 ++++ internal/cmd/profiles/profiles.go | 27 ++- internal/cmd/profiles/profiles_test.go | 135 ++++++++++++- internal/config/config.go | 91 +++++++++ internal/config/config_test.go | 65 +++++++ internal/config/relocate.go | 28 +++ internal/config/relocate_test.go | 41 ++++ internal/keychain/keychain.go | 16 ++ 18 files changed, 1097 insertions(+), 144 deletions(-) diff --git a/README.md b/README.md index 0e0e955..9a26aef 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). +One desktop OAuth client can be imported into profiles in both tools, but each CLI asks for consent and stores its token separately. See [OAuth setup](WORKSPACE_ADMINS.md) for personal External/testing and organization Internal guidance, including profile-specific client imports. ## Profiles diff --git a/WORKSPACE_ADMINS.md b/WORKSPACE_ADMINS.md index 80eaf09..d72cacc 100644 --- a/WORKSPACE_ADMINS.md +++ b/WORKSPACE_ADMINS.md @@ -1,28 +1,15 @@ -# Google CLI for Workspace administrators +# OAuth setup for `gro` and `grw` -This guide sets up one organization-managed desktop OAuth client that employees can use with both `gro` and `grw`. Each user still grants consent, and each binary stores its token under a separate identity. +This guide covers OAuth clients for personal Google accounts and Google Workspace organizations. A CLI profile selects a saved account token; an OAuth client identifies the app shown during consent. Each profile can use its own OAuth client, and each CLI stores its token separately. -## Before you begin +## Choose an audience -You need: +- **Personal Google account:** use an **External** app. Testing is the simplest initial setup: add your account as a test user and expect a testing warning. Because these CLIs request scopes beyond basic profile information, refresh tokens for an External app in Testing expire after seven days. An External app in Production avoids this Testing-specific limit, but unverified warnings and the 100-user cap can still apply. Personal-only apps or apps for a few personally known users may qualify for a verification exemption; review Google's current requirements before publishing. +- **Google Workspace organization:** a Workspace admin can create an **Internal** app for accounts in the project's organization. This avoids the External-app verification path, while Workspace admin policies still apply. The project must belong to the organization for Internal to be available. -- A Google Workspace administrator account. -- Permission to create a Google Cloud project under the Workspace organization. -- A controlled channel for distributing the downloaded OAuth client JSON. +## Required APIs and scopes -An Internal audience limits consent to accounts in the Workspace organization and avoids the External-app verification path. If the project is not attached to the organization, the Internal option will not appear. - -## Create the project and client - -1. In [Google Cloud Console](https://console.cloud.google.com/), create or select a project under the Workspace organization. -2. Enable Gmail API, Google Calendar API, People API, and Google Drive API. -3. In Google Auth Platform (or OAuth consent screen), set the audience to **Internal** and provide the application/support details. -4. Add the union of scopes below under Data Access. -5. Create an OAuth client with application type **Desktop app** and download its JSON. - -One desktop client is sufficient because the binaries request scopes at authorization time. Users authorize each binary independently. - -## Scope inventory +Enable Gmail API, Google Calendar API, People API, and Google Drive API for the APIs your CLI uses. If both binaries will be used, enable all four and declare the union of scopes below in Google Auth Platform's Data Access settings. ### `gro` @@ -55,28 +42,50 @@ https://www.googleapis.com/auth/drive.metadata Gmail settings access supports filters. The broad mail scope is required for permanent deletion; the command defaults to recoverable Trash and gates permanent deletion behind `--permanent --yes`. Calendar scopes support reading and mutating events, the Contacts scope supports reading and mutating contacts and groups, the Drive scopes support reading, uploading, organizing, trashing, restoring, and permanently deleting files, and the profile scope supports `grw me`. -## Distribute and verify +## Personal account: External app + +1. In [Google Cloud Console](https://console.cloud.google.com/), create or select a project you control. +2. Enable the required APIs above. +3. In Google Auth Platform's **Branding** page, enter the app name, user support email, and developer contact email. Under **Audience**, choose **External**. For the simplest initial setup, leave publishing status at **Testing** and add your Google account under **Test users**. +4. Add the scopes needed by your CLI under **Data Access**. +5. Create an OAuth client with application type **Desktop app** and download its JSON file. -Distribute the OAuth client JSON through an access-controlled vault, MDM, or internal file service. Desktop OAuth client material identifies the application; user refresh tokens are separate secrets and must remain in each user's selected keyring backend. +The testing warning is expected, and reauthorization is required seven days after each authorization. Moving an External app to Production removes this Testing-specific token limit, but other token expiration rules still apply. Publishing and verification are separate steps: personal-only use or a few personally known users may qualify for a verification exemption, while unverified warnings and the 100-user cap can remain. If **Publish app** is unavailable, complete the Branding requirements shown in Google Auth Platform, including any required app homepage, privacy policy, and terms links. Review Google's current [personal-use and branding requirements](https://developers.google.com/identity/protocols/oauth2/production-readiness/brand-verification#personal-use), [OAuth app states](https://developers.google.com/identity/protocols/oauth2/production-readiness/overview), [Gmail scope requirements](https://developers.google.com/workspace/gmail/api/auth/scopes), and [restricted-scope verification](https://developers.google.com/identity/protocols/oauth2/production-readiness/restricted-scope-verification) before distributing an app more broadly. -Each user can give the JSON to the setup wizard from a file, clipboard, or terminal paste: +Import the client for the profile you want to authorize: ```bash -gro init --credentials-file /path/to/oauth-client.json -gro me +grw --profile personal init --credentials-file /path/to/oauth-client.json +grw --profile personal me -grw init --credentials-file /path/to/oauth-client.json -grw mail list --max 1 +# If you also use gro, import the same client for its separate profile/token: +gro --profile personal init --credentials-file /path/to/oauth-client.json ``` -If one binary is already configured, the other setup wizard can discover and reuse its OAuth client JSON. It does not reuse the token: the user sees a separate consent flow for the other identity and scope set. +## Workspace organization: Internal app + +You need a Google Workspace administrator account, permission to create a Google Cloud project under the organization, and a controlled channel for distributing the OAuth client JSON. + +1. In Google Cloud Console, create or select a project under the Workspace organization. +2. Enable the required APIs above. +3. In Google Auth Platform, set the audience to **Internal**, provide the app and support details, and add the union of scopes above under **Data Access**. +4. Create an OAuth client with application type **Desktop app** and download its JSON. + +Distribute the JSON through an access-controlled vault, MDM, or internal file service. Each CLI still asks the user for consent and keeps its token in a separate keyring namespace. Use `gro init --credentials-file /path/to/oauth-client.json` and `grw init --credentials-file /path/to/oauth-client.json`, adding `--profile ` before `init` when authorizing named profiles. + +## Profile-specific OAuth clients + +`--credentials-file` imports the JSON for the selected CLI profile and stores it in a profile-specific managed file. For example, `grw --profile work init --credentials-file ...` associates the client with `google-readwrite/work`; it does not replace the legacy shared client or another profile's client. Run a matching `gro` command to import that JSON for a `gro` profile. The automatic sibling-client lookup applies to the legacy shared `oauth_client.json`; profile-specific imports are not discovered by the other binary. + +If the selected profile already has a token, re-importing the same OAuth client ID is allowed. Importing a different client ID is refused so the existing token is not used with the wrong app. Use a new profile, or clear only the selected profile's token with the same `--profile` selector before authorizing a different client. ## Administration and troubleshooting -- If users see an unverified-app warning, confirm the project belongs to the Workspace organization and the audience is Internal. -- If access is blocked, use Admin Console → Security → Access and data control → API controls to trust or allow the OAuth client for the intended organizational units or groups. +- If a personal-account user cannot consent while the app is in Testing, confirm that their Google account is listed under **Test users**. +- If Google returns `org_internal`, the OAuth app is restricted to the Workspace organization that owns the project. Use an External app/client for a personal Google account; CLI `--profile` names do not change an OAuth app's audience. +- If Workspace access is blocked, use Admin Console → Security → Access and data control → API controls to trust or allow the OAuth client for the intended organizational units or groups. - If an API reports `SERVICE_DISABLED`, enable the named API in the Cloud project and wait for propagation. - Revoke a user's grant through Google Account permissions, or block the client in Admin Console to revoke organization access. - Rotate and redistribute the client JSON if its distribution boundary is breached; test the rotation with one user first. -For personal accounts or cross-organization distribution, create an External-audience client and follow Google's current verification requirements. See Google's guidance on [OAuth app audience](https://support.google.com/cloud/answer/15549945), [when verification is not needed](https://support.google.com/cloud/answer/13464323), and [installed applications](https://developers.google.com/identity/protocols/oauth2/native-app). +See Google's guides for [app audiences and test users](https://support.google.com/cloud/answer/15549945), [OAuth 2.0 for desktop apps](https://developers.google.com/identity/protocols/oauth2/native-app), and [refresh-token expiration](https://developers.google.com/identity/protocols/oauth2). diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5d6d16d..c4936d5 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -57,15 +57,26 @@ 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 JSON associated with the effective +// credential ref, falling back to the legacy shared path when that profile +// has no own association. func GetOAuthConfig() (*oauth2.Config, error) { + ref, err := keychain.ResolveEffectiveCredentialRef() + if err != nil { + return nil, err + } + return GetOAuthConfigForRef(ref) +} + +// GetOAuthConfigForRef loads the deployment-material OAuth client JSON +// associated with ref, using the legacy shared path if no profile entry +// exists. The OAuth client is never stored in the keyring. +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) 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 +117,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 +131,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 { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 6cb28ed..2eb8908 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1,14 +1,171 @@ 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 TestGetOAuthConfigForRefSelectsProfileClient(t *testing.T) { + credtest.Setup(t) + defaultPath := filepath.Join(credtest.ConfigDir(t), "default.json") + profilePath := filepath.Join(credtest.ConfigDir(t), "profile.json") + if err := os.WriteFile(defaultPath, []byte(testOAuthClientJSON("default-client", "https://oauth2.googleapis.com/token")), 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(profilePath, []byte(testOAuthClientJSON("profile-client", "https://oauth2.googleapis.com/token")), 0600); err != nil { + t.Fatal(err) + } + const ref = "google-readonly/personal" + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + OAuthClientPath: defaultPath, + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + ref: {OAuthClientPath: profilePath}, + }, + }); err != nil { + t.Fatal(err) + } + + got, err := GetOAuthConfigForRef(ref) + if err != nil { + t.Fatalf("GetOAuthConfigForRef: %v", err) + } + if got.ClientID != "profile-client" { + t.Fatalf("profile ClientID = %q, want profile-client", got.ClientID) + } + legacy, err := GetOAuthConfigForRef(config.DefaultCredentialRef) + if err != nil { + t.Fatalf("GetOAuthConfigForRef(default): %v", err) + } + if legacy.ClientID != "default-client" { + t.Fatalf("legacy ClientID = %q, want default-client", legacy.ClientID) + } +} + +func TestGetHTTPClientForRefRefreshesWithMatchingOAuthClient(t *testing.T) { + credtest.Setup(t) + keychain.SetCredentialRefOverride("", false) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + const profileRef = "google-readonly/personal" + + refreshClientID := 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() + } + refreshClientID <- clientID + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"access_token":"refreshed-profile-token","token_type":"Bearer","expires_in":3600}`) + case "/api": + if got := r.Header.Get("Authorization"); got != "Bearer refreshed-profile-token" { + http.Error(w, "wrong bearer token", http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusNoContent) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + configDir := credtest.ConfigDir(t) + defaultPath := filepath.Join(configDir, "default.json") + profilePath := filepath.Join(configDir, "profile.json") + if err := os.WriteFile(defaultPath, []byte(testOAuthClientJSON("default-client", server.URL+"/token")), 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(profilePath, []byte(testOAuthClientJSON("profile-client", server.URL+"/token")), 0600); err != nil { + t.Fatal(err) + } + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + OAuthClientPath: defaultPath, + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + profileRef: {OAuthClientPath: profilePath}, + }, + }); err != nil { + t.Fatal(err) + } + + seed := func(ref string, token *oauth2.Token) { + t.Helper() + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatalf("OpenRef(%s): %v", ref, err) + } + if err := st.SetToken(token); err != nil { + _ = st.Close() + t.Fatalf("SetToken(%s): %v", ref, err) + } + if err := st.Close(); err != nil { + t.Fatalf("Close(%s): %v", ref, err) + } + } + seed(config.DefaultCredentialRef, &oauth2.Token{AccessToken: "unchanged-default", RefreshToken: "default-refresh", TokenType: "Bearer"}) + seed(profileRef, &oauth2.Token{AccessToken: "expired-profile", RefreshToken: "profile-refresh", TokenType: "Bearer", Expiry: time.Now().Add(-time.Hour)}) + + client, err := GetHTTPClientForRef(context.Background(), profileRef) + if err != nil { + t.Fatalf("GetHTTPClientForRef: %v", err) + } + resp, err := client.Get(server.URL + "/api") + if err != nil { + t.Fatalf("GET api: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("GET api status = %d, want %d", resp.StatusCode, http.StatusNoContent) + } + select { + case got := <-refreshClientID: + if got != "profile-client" { + t.Fatalf("refresh client_id = %q, want profile-client", got) + } + case <-time.After(2 * time.Second): + t.Fatal("profile token was not refreshed") + } + + assertToken := func(ref, want string) { + t.Helper() + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatal(err) + } + tok, err := st.Token() + _ = st.Close() + if err != nil || tok.AccessToken != want { + t.Fatalf("token at %s = (%q, %v), want %q", ref, tok.AccessToken, err, want) + } + } + assertToken(profileRef, "refreshed-profile-token") + assertToken(config.DefaultCredentialRef, "unchanged-default") +} + +func testOAuthClientJSON(clientID, tokenURL string) string { + return fmt.Sprintf(`{"installed":{"client_id":%q,"project_id":"test","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":%q,"auth_provider_x509_cert_url":"https://www.googleapis.com/oauth2/v1/certs","client_secret":"test-secret","redirect_uris":["http://localhost"]}}`, clientID, tokenURL) +} + // 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/cmd/config/behavior_test.go b/internal/cmd/config/behavior_test.go index c3638dc..fc2f568 100644 --- a/internal/cmd/config/behavior_test.go +++ b/internal/cmd/config/behavior_test.go @@ -107,6 +107,65 @@ func TestRunShowReportsState(t *testing.T) { } } +func TestRunShowUsesSelectedProfileClient(t *testing.T) { + credtest.Setup(t) + keychain.SetCredentialRefOverride("", false) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + t.Setenv(keychain.CredentialRefEnvVar(), "") + const ref = "google-readonly/personal" + dir := credtest.ConfigDir(t) + defaultPath := filepath.Join(dir, "shared-client.json") + profilePath := filepath.Join(dir, "personal-client.json") + profileJSON := strings.ReplaceAll(clientJSON, "123.apps.googleusercontent.com", "456.apps.googleusercontent.com") + if err := os.WriteFile(defaultPath, []byte(clientJSON), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(profilePath, []byte(profileJSON), 0o600); err != nil { + t.Fatal(err) + } + if err := appconfig.SaveConfig(&appconfig.Config{ + CredentialRef: appconfig.DefaultCredentialRef, + OAuthClientPath: defaultPath, + ProfileOAuth: map[string]appconfig.ProfileOAuthConfig{ + ref: {OAuthClientPath: profilePath}, + }, + }); err != nil { + t.Fatal(err) + } + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatal(err) + } + if err := st.SetToken(&oauth2.Token{AccessToken: "profile-token", RefreshToken: "profile-refresh"}); err != nil { + _ = st.Close() + t.Fatal(err) + } + if err := st.Close(); err != nil { + t.Fatal(err) + } + keychain.SetCredentialRefOverride(ref, true) + + jsonOut := capture(t, func() { + if err := runShow(true, false); err != nil { + t.Errorf("runShow: %v", err) + } + }) + var status showStatus + if err := json.Unmarshal([]byte(jsonOut), &status); err != nil { + t.Fatalf("show --json: %v\n%s", err, jsonOut) + } + if status.CredentialRef != ref { + t.Fatalf("credential_ref = %q, want %q", status.CredentialRef, ref) + } + if status.OAuthClientPath != appconfig.ShortenPath(profilePath) { + t.Fatalf("OAuthClientPath = %q, want selected profile path %q", status.OAuthClientPath, appconfig.ShortenPath(profilePath)) + } + wantFingerprint := "sha256:" + fileFingerprint([]byte(profileJSON)) + if status.OAuthClientFingerprint != wantFingerprint { + t.Fatalf("profile OAuth fingerprint = %q, want %q", status.OAuthClientFingerprint, wantFingerprint) + } +} + // TestRunShowReportsKeyringBackendSelector seeds a non-empty // cfg.Keyring.Backend and asserts both the text output and the JSON // status carry the selector value — proving the new `keyring.backend: diff --git a/internal/cmd/config/config.go b/internal/cmd/config/config.go index 355042e..7ff9245 100644 --- a/internal/cmd/config/config.go +++ b/internal/cmd/config/config.go @@ -165,13 +165,14 @@ 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(cfg.OAuthClientPathForRef(st.Ref())), OAuthClientPresent: false, } + clientPath := cfg.OAuthClientPathForRef(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 + if data, rerr := os.ReadFile(clientPath); rerr == nil { //nolint:gosec // deployment-material path status.OAuthClientPresent = true status.OAuthClientFingerprint = "sha256:" + fileFingerprint(data) if verbose { diff --git a/internal/cmd/init/init.go b/internal/cmd/init/init.go index e2d036c..63e914c 100644 --- a/internal/cmd/init/init.go +++ b/internal/cmd/init/init.go @@ -66,9 +66,9 @@ The wizard first asks how you're getting your credentials.json: enabling the required Google APIs (shown for your CLI during setup), and downloading OAuth 2.0 Desktop-app credentials. -If you're a Google Workspace admin and want to set up one Internal OAuth app -for your whole org, see: - ` + workspaceAdminsURL + ` +For guidance on personal External apps, Workspace Internal apps, and +profile-specific OAuth clients, see: + ` + oauthSetupURL + ` 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. @@ -108,11 +108,13 @@ type initDeps struct { DiscoverSiblingClientJSON func() (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(ref string) (string, error) + NewProfileOAuthClientPath 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 + RemoveFile func(path string) error + Stat func(path string) (os.FileInfo, error) // Clipboard. Supported is checked first; ReadAll only called if Supported. ClipboardSupported func() bool @@ -156,7 +158,7 @@ type initDeps struct { // OAuth. ExchangeAuthCode func(ctx context.Context, cfg *oauth2.Config, code string) (*oauth2.Token, error) - GetOAuthConfig func() (*oauth2.Config, error) + GetOAuthConfig 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 @@ -191,34 +193,40 @@ func defaultDeps() initDeps { return initDeps{ View: view.New(), DiscoverSiblingClientJSON: config.SiblingOAuthClientPath, - // 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) { + // The OAuth client JSON is deployment material (§1.2): older installs + // use the shared path, while explicitly imported profile clients resolve + // to their own managed JSON file. + GetCredentialsPath: func(ref string) (string, error) { cfg, err := config.LoadConfigForRuntime() if err != nil { return "", err } - return config.ExpandPath(cfg.OAuthClientPath), nil + if ref == "" { + ref = cfg.CredentialRef + } + return cfg.OAuthClientPathForRef(ref), nil }, - ReadFile: os.ReadFile, - WriteFile: os.WriteFile, - Chmod: os.Chmod, - Stat: os.Stat, - ClipboardSupported: func() bool { return !clipboard.Unsupported }, - ClipboardReadAll: clipboard.ReadAll, - OpenBrowser: browser.OpenURL, - DetectConfigRelocation: config.DetectConfigRelocation, - ApplyConfigRelocation: config.ApplyConfigRelocation, - EnsureMigrated: ensureMigrated, - DescribeTarget: describeTarget, - RecordIdentity: recordIdentity, - HasStoredToken: storeHasToken, - SetToken: storeSetToken, - DeleteToken: storeDeleteToken, - GetStorageBackend: storeBackendLabel, - StdinReadAll: readAllStdin, - ExchangeAuthCode: auth.ExchangeAuthCode, - GetOAuthConfig: auth.GetOAuthConfig, + NewProfileOAuthClientPath: config.NewProfileOAuthClientPath, + ReadFile: os.ReadFile, + WriteFile: os.WriteFile, + Chmod: os.Chmod, + RemoveFile: os.Remove, + Stat: os.Stat, + ClipboardSupported: func() bool { return !clipboard.Unsupported }, + ClipboardReadAll: clipboard.ReadAll, + OpenBrowser: browser.OpenURL, + DetectConfigRelocation: config.DetectConfigRelocation, + ApplyConfigRelocation: config.ApplyConfigRelocation, + EnsureMigrated: ensureMigrated, + DescribeTarget: describeTarget, + RecordIdentity: recordIdentity, + HasStoredToken: storeHasToken, + SetToken: storeSetToken, + DeleteToken: storeDeleteToken, + GetStorageBackend: storeBackendLabel, + StdinReadAll: readAllStdin, + ExchangeAuthCode: auth.ExchangeAuthCode, + GetOAuthConfig: auth.GetOAuthConfigForRef, GmailVerify: func(ctx context.Context) (string, error) { c, err := gmail.NewClient(ctx) if err != nil { @@ -425,18 +433,18 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { target = "the active profile" } - credPath, err := d.GetCredentialsPath() + credPath, err := d.GetCredentialsPath(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 := ensureCredentials(d, opts, credPath, targetRef); err != nil { return err } // Step 3: token resolution. - handled, err := tryExistingToken(ctx, d, opts, target) + handled, err := tryExistingToken(ctx, d, opts, target, targetRef) if err != nil { return err } @@ -445,7 +453,7 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { } // Step 4: OAuth flow. - oauthCfg, err := d.GetOAuthConfig() + oauthCfg, err := d.GetOAuthConfig(targetRef) if err != nil { return fmt.Errorf("loading OAuth config: %w", err) } @@ -497,14 +505,23 @@ 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 granted scopes. If config cannot be read after token + // exchange, do not replace it with an empty config: that could discard + // other profile-specific OAuth-client bindings. 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: token was saved, but granted scopes could not be recorded because config could not be loaded: %v", cfgErr) + } else { + if targetRef != "" { + profile := cfg.ProfileOAuth[targetRef] + profile.GrantedScopes = config.Scopes() + cfg.SetProfileOAuth(targetRef, profile) + } 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 @@ -625,7 +642,7 @@ func apisForScopes(scopes []string) []string { // 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 tryExistingToken(ctx context.Context, d initDeps, opts *initOptions, target, targetRef string) (bool, error) { if !d.HasStoredToken() { return false, nil } @@ -634,7 +651,7 @@ 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 != "" { + if msg := auth.CheckScopesMigration(cfg.GrantedScopesForRef(targetRef)); msg != "" { d.View.Error("Recorded scopes are stale.") d.View.Println(msg) if err := promptAndDeleteForReauth(d, opts, target); err != nil { @@ -736,14 +753,14 @@ func finishExisting(d initDeps, profile *people.Profile) error { // ensureCredentials makes sure credentials.json exists at credPath, populating // it from --credentials-file or the interactive wizard if needed. -func ensureCredentials(d initDeps, opts *initOptions, credPath string) error { +func ensureCredentials(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) } if _, err := d.Stat(credPath); err == nil { @@ -780,10 +797,14 @@ func ensureCredentials(d initDeps, opts *initOptions, credPath string) error { d.View.Println(" 3. Create OAuth 2.0 Desktop-app credentials.") d.View.Println(" 4. Copy the JSON to your clipboard, OR download the JSON file.") d.View.Println("") - d.View.Println("Optional: publish your OAuth app to avoid 7-day token expiry.") + d.View.Println("Personal account: enter the app name, user support email, and developer contact email in Branding. Choose External under Audience.") + d.View.Println("Add your Google account under Test users.") + d.View.Println("Testing is simplest to start, but this CLI's refresh tokens expire after 7 days in Testing.") + d.View.Println("Production removes this Testing-specific limit, but unverified warnings and a 100-user cap may still apply.") + d.View.Println("Personal-only apps or apps for a few personally known people may qualify for a verification exemption. If Publish app is disabled, complete the Branding fields Google requests.") d.View.Println("") - d.View.Println("Workspace admin? Set up an Internal OAuth app once for your whole org:") - d.View.Println(" " + workspaceAdminsURL) + d.View.Println("For personal and Workspace setup details, see:") + d.View.Println(" " + oauthSetupURL) default: return fmt.Errorf("unknown audience: %s", audience) } @@ -842,6 +863,92 @@ func ensureCredentials(d initDeps, opts *initOptions, credPath string) error { return errors.New("could not obtain valid credentials.json after 3 attempts") } +// importProfileFromFile stores an explicit OAuth client import at a unique +// managed path, then associates only the selected credential ref with it. +// The legacy shared file is never overwritten by --credentials-file. +func importProfileFromFile(d initDeps, srcPath, targetRef string) error { + blob, err := d.ReadFile(srcPath) + if err != nil { + return fmt.Errorf("reading %s: %w", srcPath, err) + } + imported, err := google.ConfigFromJSON(blob, config.Scopes()...) + if err != nil { + return fmt.Errorf("invalid OAuth client JSON: %w", err) + } + + cfg, err := d.LoadConfig() + if err != nil { + return fmt.Errorf("loading profile config: %w", err) + } + ref := targetRef + if ref == "" { + ref = cfg.CredentialRef + } + if ref == "" { + ref = config.DefaultCredentialRef + } + if ref == "" { + return errors.New("could not resolve the target credential profile") + } + + hasToken := d.HasStoredToken != nil && d.HasStoredToken() + var grantedScopes []string + if hasToken { + if d.GetOAuthConfig == nil { + return storedTokenClientChangeError(ref, "the current OAuth client could not be verified", nil) + } + current, cfgErr := d.GetOAuthConfig(ref) + if cfgErr != nil { + return storedTokenClientChangeError(ref, "the current OAuth client could not be verified", cfgErr) + } + if current == nil || current.ClientID == "" || current.ClientID != imported.ClientID { + return storedTokenClientChangeError(ref, "the imported OAuth client has a different client ID", nil) + } + grantedScopes = cfg.GrantedScopesForRef(ref) + } + + getPath := d.NewProfileOAuthClientPath + if getPath == nil { + getPath = config.NewProfileOAuthClientPath + } + managedPath, err := getPath() + if err != nil { + return fmt.Errorf("creating profile OAuth client path: %w", err) + } + if err := writeCredentials(d, managedPath, blob); err != nil { + _ = removeProfileOAuthClientFile(d, managedPath) + return err + } + cfg.SetProfileOAuth(ref, config.ProfileOAuthConfig{ + OAuthClientPath: managedPath, + GrantedScopes: grantedScopes, + }) + if err := d.SaveConfig(cfg); err != nil { + if removeErr := removeProfileOAuthClientFile(d, managedPath); removeErr != nil { + return fmt.Errorf("saving OAuth client association for %s: %w (could not remove staged client file: %w)", ref, err, removeErr) + } + return fmt.Errorf("saving OAuth client association for %s: %w", ref, err) + } + d.View.Success("OAuth client JSON saved for %s", ref) + return nil +} + +func storedTokenClientChangeError(ref, reason string, cause error) error { + msg := fmt.Sprintf("profile %s already has a stored token; refusing to bind another OAuth client because %s. Run '%s config clear' with the same --profile/credential-ref selection to remove only this profile's token, then retry, or use a new profile", + ref, reason, config.ProductName()) + if cause != nil { + return fmt.Errorf("%s: %w", msg, cause) + } + return errors.New(msg) +} + +func removeProfileOAuthClientFile(d initDeps, path string) error { + if d.RemoveFile != nil { + return d.RemoveFile(path) + } + return os.Remove(path) +} + // importFromFile reads, validates, and writes credentials.json from a path. func importFromFile(d initDeps, srcPath, dstPath string) error { blob, err := d.ReadFile(srcPath) @@ -900,10 +1007,10 @@ func extractAuthCode(input string) string { return input } -// workspaceAdminsURL points to the repo's Workspace-admin walkthrough. +// oauthSetupURL points to the personal and Workspace OAuth setup guide. // Referenced from both cmd.Long and the runtime wizard, so installed-CLI // users (Homebrew/Chocolatey/Winget) reach it without a local checkout. -const workspaceAdminsURL = "https://github.com/open-cli-collective/google-readonly/blob/main/WORKSPACE_ADMINS.md" +const oauthSetupURL = "https://github.com/open-cli-collective/google-cli/blob/main/WORKSPACE_ADMINS.md" // huhPrompter is the production prompter — wraps huh. type huhPrompter struct{} diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index 486e269..5378f73 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -211,11 +211,21 @@ func baseDeps(t *testing.T, fs *fakeFS) initDeps { t.Helper() credPath := filepath.Join(t.TempDir(), "credentials.json") configPath := filepath.Join(filepath.Dir(credPath), "config.json") + profileClientPath := filepath.Join(filepath.Dir(credPath), "oauth-client-profile-test.json") cfgPtr := &config.Config{} return initDeps{ - View: view.NewWithWriters(&bytes.Buffer{}, &bytes.Buffer{}), - GetCredentialsPath: func() (string, error) { return credPath, nil }, + View: view.NewWithWriters(&bytes.Buffer{}, &bytes.Buffer{}), + GetCredentialsPath: func(_ string) (string, error) { return credPath, nil }, + NewProfileOAuthClientPath: func() (string, error) { return profileClientPath, nil }, + RemoveFile: func(path string) error { + delete(fs.files, path) + delete(fs.perms, path) + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return err + } + return nil + }, ReadFile: func(p string) ([]byte, error) { if b, err := fs.ReadFile(p); err == nil { return b, nil @@ -237,16 +247,21 @@ func baseDeps(t *testing.T, fs *fakeFS) initDeps { ClipboardReadAll: func() (string, error) { return "", errors.New("disabled") }, OpenBrowser: func(_ string) error { return nil }, EnsureMigrated: func() error { return nil }, - HasStoredToken: func() bool { return false }, - SetToken: func(_ *oauth2.Token) error { return nil }, - DeleteToken: func() error { return nil }, - GetStorageBackend: func() string { return "test" }, - StdinReadAll: func() (string, error) { return "", nil }, + DescribeTarget: func() (string, string, string) { + return config.DefaultCredentialRef, "config.yml", "" + }, + HasStoredToken: func() bool { return false }, + SetToken: func(_ *oauth2.Token) error { return nil }, + DeleteToken: func() error { return nil }, + GetStorageBackend: func() string { return "test" }, + StdinReadAll: func() (string, error) { return "", nil }, ExchangeAuthCode: func(_ context.Context, _ *oauth2.Config, _ string) (*oauth2.Token, error) { return &oauth2.Token{AccessToken: "tok"}, nil }, - GetOAuthConfig: func() (*oauth2.Config, error) { return &oauth2.Config{}, nil }, - GmailVerify: func(_ context.Context) (string, error) { return "ada@example.com", nil }, + GetOAuthConfig: func(string) (*oauth2.Config, error) { + return &oauth2.Config{ClientID: "1234.apps.googleusercontent.com"}, nil + }, + GmailVerify: func(_ context.Context) (string, error) { return "ada@example.com", nil }, PeopleGetMe: func(_ context.Context) (*people.Profile, error) { return &people.Profile{ResourceName: "people/c1", DisplayName: "Ada", PrimaryEmail: "ada@example.com"}, nil }, @@ -266,23 +281,36 @@ func TestEnsureCredentialsFlagFile(t *testing.T) { if err := os.WriteFile(src, []byte(validOAuthJSON), 0644); err != nil { t.Fatal(err) } - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) + profilePath, _ := d.NewProfileOAuthClientPath() + fs.files[dst] = []byte("shared-client-must-remain-unchanged") + fs.perms[dst] = 0600 stub := &stubPrompter{} d.Prompter = stub - if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } - got, err := fs.ReadFile(dst) + got, err := fs.ReadFile(profilePath) if err != nil { - t.Fatalf("dst not written: %v", err) + t.Fatalf("profile client not written: %v", err) } if string(got) != validOAuthJSON { - t.Errorf("dst content mismatch") + t.Errorf("profile client content mismatch") } - if perm := fs.perms[dst]; perm != 0600 { + if perm := fs.perms[profilePath]; perm != 0600 { t.Errorf("expected perms 0600, got %o", perm) } + if string(fs.files[dst]) != "shared-client-must-remain-unchanged" { + t.Fatal("--credentials-file overwrote the legacy shared OAuth client") + } + cfg, err := d.LoadConfig() + if err != nil { + t.Fatal(err) + } + if got := cfg.ProfileOAuth[config.DefaultCredentialRef].OAuthClientPath; got != profilePath { + t.Fatalf("profile client association = %q, want %q", got, profilePath) + } // --credentials-file bypasses the wizard entirely; audience must not be asked. if contains(stub.calls, "audience") { t.Errorf("expected --credentials-file bypass; SelectAudience was called: calls=%v", stub.calls) @@ -298,22 +326,94 @@ func TestEnsureCredentialsRejectsBadJSON(t *testing.T) { if err := os.WriteFile(src, []byte("garbage"), 0644); err != nil { t.Fatal(err) } - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{} - if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst); err == nil { + if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst, config.DefaultCredentialRef); err == nil { t.Fatal("expected error for invalid JSON") } } +func TestEnsureCredentialsRefusesDifferentClientWhenTokenExists(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + sharedPath, _ := d.GetCredentialsPath(config.DefaultCredentialRef) + profilePath, _ := d.NewProfileOAuthClientPath() + const oldPath = "/existing/profile-client.json" + const oldScopes = "https://www.googleapis.com/auth/gmail.readonly" + cfg := &config.Config{CredentialRef: config.DefaultCredentialRef} + cfg.SetProfileOAuth(config.DefaultCredentialRef, config.ProfileOAuthConfig{ + OAuthClientPath: oldPath, + GrantedScopes: []string{oldScopes}, + }) + d.LoadConfig = func() (*config.Config, error) { return cfg, nil } + d.HasStoredToken = func() bool { return true } + d.GetOAuthConfig = func(string) (*oauth2.Config, error) { + return &oauth2.Config{ClientID: "old-client.apps.googleusercontent.com"}, nil + } + fs.files[sharedPath] = []byte("legacy-shared-client") + + src := filepath.Join(t.TempDir(), "downloaded.json") + if err := os.WriteFile(src, []byte(validOAuthJSON), 0600); err != nil { + t.Fatal(err) + } + err := ensureCredentials(d, &initOptions{credentialsFile: src}, sharedPath, config.DefaultCredentialRef) + if err == nil || !strings.Contains(err.Error(), "different client ID") { + t.Fatalf("import with existing token = %v, want different-client error", err) + } + if got := cfg.OAuthClientPathForRef(config.DefaultCredentialRef); got != oldPath { + t.Fatalf("profile OAuth path changed to %q, want %q", got, oldPath) + } + if _, err := fs.ReadFile(profilePath); err == nil { + t.Fatal("staged profile OAuth JSON remained after refusing import") + } + if got := string(fs.files[sharedPath]); got != "legacy-shared-client" { + t.Fatalf("shared OAuth client changed to %q", got) + } +} + +func TestEnsureCredentialsSameClientPreservesExistingProfileScopes(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + sharedPath, _ := d.GetCredentialsPath(config.DefaultCredentialRef) + profilePath, _ := d.NewProfileOAuthClientPath() + const oldScopes = "https://www.googleapis.com/auth/gmail.readonly" + cfg := &config.Config{CredentialRef: config.DefaultCredentialRef} + cfg.SetProfileOAuth(config.DefaultCredentialRef, config.ProfileOAuthConfig{ + OAuthClientPath: "/existing/profile-client.json", + GrantedScopes: []string{oldScopes}, + }) + d.LoadConfig = func() (*config.Config, error) { return cfg, nil } + d.HasStoredToken = func() bool { return true } + d.GetOAuthConfig = func(string) (*oauth2.Config, error) { + return &oauth2.Config{ClientID: "1234.apps.googleusercontent.com"}, nil + } + src := filepath.Join(t.TempDir(), "downloaded.json") + if err := os.WriteFile(src, []byte(validOAuthJSON), 0600); err != nil { + t.Fatal(err) + } + if err := ensureCredentials(d, &initOptions{credentialsFile: src}, sharedPath, config.DefaultCredentialRef); err != nil { + t.Fatalf("same-client import: %v", err) + } + profile := cfg.ProfileOAuth[config.DefaultCredentialRef] + if profile.OAuthClientPath != profilePath { + t.Fatalf("profile client path = %q, want %q", profile.OAuthClientPath, profilePath) + } + if len(profile.GrantedScopes) != 1 || profile.GrantedScopes[0] != oldScopes { + t.Fatalf("profile scopes = %v, want preserved %q", profile.GrantedScopes, oldScopes) + } +} + func TestEnsureCredentialsClipboardWizard(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) d.ClipboardSupported = func() bool { return true } d.ClipboardReadAll = func() (string, error) { return validOAuthJSON, nil } - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{credChoice: "clipboard"} - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } if _, err := fs.ReadFile(dst); err != nil { @@ -325,9 +425,9 @@ func TestEnsureCredentialsPasteWizard(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON} - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } if _, err := fs.ReadFile(dst); err != nil { @@ -345,10 +445,10 @@ func TestEnsureCredentialsFileWizard(t *testing.T) { if err := os.WriteFile(src, []byte(validOAuthJSON), 0644); err != nil { t.Fatal(err) } - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{credChoice: "file", filePath: src} - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } got, err := fs.ReadFile(dst) @@ -367,7 +467,7 @@ func TestEnsureCredentialsShortCircuitsWhenAlreadyPresent(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) // Pre-populate credentials.json on real disk so Stat finds it. if err := os.WriteFile(dst, []byte(validOAuthJSON), 0600); err != nil { @@ -377,7 +477,7 @@ func TestEnsureCredentialsShortCircuitsWhenAlreadyPresent(t *testing.T) { stub := &stubPrompter{} d.Prompter = stub - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } // SelectCredSource (the wizard's first prompt) must NOT have fired. @@ -396,10 +496,10 @@ func TestEnsureCredentialsAudienceAdminSkipsDIYSteps(t *testing.T) { d := baseDeps(t, fs) out := &bytes.Buffer{} d.View = view.NewWithWriters(out, &bytes.Buffer{}) - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{audience: "admin", credChoice: "paste", pasteJSON: validOAuthJSON} - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } got := out.String() @@ -411,24 +511,29 @@ func TestEnsureCredentialsAudienceAdminSkipsDIYSteps(t *testing.T) { } } -func TestEnsureCredentialsAudienceDIYShowsDIYStepsAndAdminPointer(t *testing.T) { +func TestEnsureCredentialsAudienceDIYShowsPersonalOAuthGuidance(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) out := &bytes.Buffer{} d.View = view.NewWithWriters(out, &bytes.Buffer{}) - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{audience: "diy", credChoice: "paste", pasteJSON: validOAuthJSON} - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } got := out.String() if !strings.Contains(got, "Enable APIs: Gmail") { t.Errorf("diy audience should show DIY steps; output:\n%s", got) } - if !strings.Contains(got, "https://github.com/open-cli-collective/google-readonly/blob/main/WORKSPACE_ADMINS.md") { - t.Errorf("diy audience should show the fully-qualified Workspace admin doc URL (so Homebrew/Choco users can reach it); output:\n%s", got) + if !strings.Contains(got, "Choose External") || + !strings.Contains(got, "refresh tokens expire after 7 days in Testing") || + !strings.Contains(got, "verification exemption") { + t.Errorf("diy audience should explain the personal External/testing setup; output:\n%s", got) + } + if !strings.Contains(got, "https://github.com/open-cli-collective/google-cli/blob/main/WORKSPACE_ADMINS.md") { + t.Errorf("diy audience should show the fully-qualified OAuth setup guide URL; output:\n%s", got) } } @@ -451,11 +556,11 @@ func TestEnsureCredentialsAudienceIsAskedOncePerWizard(t *testing.T) { idx++ return s, nil } - dst, _ := d.GetCredentialsPath() + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) stub := &stubPrompter{audience: "admin", credChoice: "clipboard"} d.Prompter = stub - if err := ensureCredentials(d, &initOptions{}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } gotAudience, gotSelect := 0, 0 @@ -479,12 +584,18 @@ func TestEnsureCredentialsTightensPermsOnOverwrite(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) - dst, _ := d.GetCredentialsPath() - // Pre-existing 0644 file. - if err := os.WriteFile(dst, []byte("old"), 0644); err != nil { + dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) + profilePath, _ := d.NewProfileOAuthClientPath() + // Pre-existing shared and profile files; neither may be left more + // permissive when the imported JSON is written. + if err := os.WriteFile(dst, []byte("shared-old"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(profilePath, []byte("old"), 0644); err != nil { t.Fatal(err) } defer os.Remove(dst) + defer os.Remove(profilePath) srcDir := t.TempDir() src := filepath.Join(srcDir, "downloaded.json") @@ -497,16 +608,23 @@ func TestEnsureCredentialsTightensPermsOnOverwrite(t *testing.T) { d.Chmod = os.Chmod d.ReadFile = os.ReadFile d.Prompter = &stubPrompter{} - if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst); err != nil { + if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } - info, err := os.Stat(dst) + info, err := os.Stat(profilePath) if err != nil { t.Fatal(err) } if info.Mode().Perm() != 0600 { t.Errorf("expected 0600 after overwrite, got %o", info.Mode().Perm()) } + shared, err := os.ReadFile(dst) + if err != nil { + t.Fatal(err) + } + if string(shared) != "shared-old" { + t.Fatalf("shared client changed to %q", shared) + } } func TestRunWithFreshSetupSavesScopesNoTTLPrompt(t *testing.T) { @@ -540,6 +658,55 @@ func TestRunWithFreshSetupSavesScopesNoTTLPrompt(t *testing.T) { if len(cfgSeen) < 1 { t.Fatalf("expected at least one config save (scopes), got %d", len(cfgSeen)) } + last := cfgSeen[len(cfgSeen)-1] + gotScopes, wantScopes := last.GrantedScopesForRef(config.DefaultCredentialRef), config.Scopes() + if len(gotScopes) != len(wantScopes) { + t.Errorf("profile granted_scopes = %v, want %v", gotScopes, wantScopes) + } else { + for i := range wantScopes { + if gotScopes[i] != wantScopes[i] { + t.Errorf("profile granted_scopes = %v, want %v", gotScopes, wantScopes) + break + } + } + } + if len(last.GrantedScopes) != 0 { + t.Errorf("legacy global granted_scopes = %v, want no cross-profile claim", last.GrantedScopes) + } +} + +func TestRunWithConfigLoadFailureDoesNotReplaceProfileBindingsAfterTokenSave(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + credPath, err := d.GetCredentialsPath(config.DefaultCredentialRef) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(credPath, []byte(validOAuthJSON), 0600); err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + d.View = view.NewWithWriters(&out, &errOut) + d.LoadConfig = func() (*config.Config, error) { return nil, errors.New("config unavailable") } + tokenSaved := false + d.SetToken = func(*oauth2.Token) error { tokenSaved = true; return nil } + saveCalls := 0 + d.SaveConfig = func(*config.Config) error { saveCalls++; return nil } + d.Prompter = &stubPrompter{redirectURL: "http://localhost/?code=AUTH-CODE"} + + if err := runWith(context.Background(), d, &initOptions{noVerify: true}); err != nil { + t.Fatalf("runWith after successful token exchange: %v", err) + } + if !tokenSaved { + t.Fatal("expected the exchanged token to be saved") + } + if saveCalls != 0 { + t.Fatalf("SaveConfig called %d times after config load failed; want no save", saveCalls) + } + if !strings.Contains(errOut.String(), "granted scopes could not be recorded") { + t.Fatalf("stderr warning = %q, want scope-metadata warning", errOut.String()) + } } func TestRunWithExpiredTokenPromptsReauth(t *testing.T) { @@ -727,7 +894,7 @@ func TestRunWithRecordedStaleScopesReauths(t *testing.T) { fs := newFakeFS() d := baseDeps(t, fs) - credPath, _ := d.GetCredentialsPath() + credPath, _ := d.GetCredentialsPath(config.DefaultCredentialRef) if err := os.WriteFile(credPath, []byte(validOAuthJSON), 0600); err != nil { t.Fatal(err) } @@ -788,7 +955,7 @@ func TestRunWithExistingTokenStaleScopeReauths(t *testing.T) { d := baseDeps(t, fs) // Pre-populate credentials.json so we don't enter the wizard. - credPath, _ := d.GetCredentialsPath() + credPath, _ := d.GetCredentialsPath(config.DefaultCredentialRef) if err := os.WriteFile(credPath, []byte(validOAuthJSON), 0600); err != nil { t.Fatal(err) } @@ -833,7 +1000,7 @@ func TestRunWithExistingTokenNoVerifyStillCatchesStaleScopes(t *testing.T) { fs := newFakeFS() d := baseDeps(t, fs) - credPath, _ := d.GetCredentialsPath() + credPath, _ := d.GetCredentialsPath(config.DefaultCredentialRef) if err := os.WriteFile(credPath, []byte(validOAuthJSON), 0600); err != nil { t.Fatal(err) } @@ -866,7 +1033,7 @@ func TestRunWithExistingTokenNoVerifySkipsAPI(t *testing.T) { fs := newFakeFS() d := baseDeps(t, fs) - credPath, _ := d.GetCredentialsPath() + credPath, _ := d.GetCredentialsPath(config.DefaultCredentialRef) if err := os.WriteFile(credPath, []byte(validOAuthJSON), 0600); err != nil { t.Fatal(err) } diff --git a/internal/cmd/init/sibling_test.go b/internal/cmd/init/sibling_test.go index a1254ce..4794049 100644 --- a/internal/cmd/init/sibling_test.go +++ b/internal/cmd/init/sibling_test.go @@ -3,6 +3,8 @@ package initcmd import ( "path/filepath" "testing" + + "github.com/open-cli-collective/google-cli/internal/config" ) // TestEnsureCredentials_ReusesSiblingOAuthClient proves the seamless-setup @@ -20,7 +22,7 @@ func TestEnsureCredentials_ReusesSiblingOAuthClient(t *testing.T) { d.Prompter = prompter d.DiscoverSiblingClientJSON = func() (string, string, bool) { return siblingPath, "google-readonly", true } - if err := ensureCredentials(d, &initOptions{}, credPath); err != nil { + if err := ensureCredentials(d, &initOptions{}, credPath, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } if _, ok := fs.files[credPath]; !ok { @@ -42,7 +44,7 @@ func TestEnsureCredentials_NoSiblingFallsThroughToWizard(t *testing.T) { d.Prompter = prompter d.DiscoverSiblingClientJSON = func() (string, string, bool) { return "", "", false } - if err := ensureCredentials(d, &initOptions{}, credPath); err != nil { + if err := ensureCredentials(d, &initOptions{}, credPath, config.DefaultCredentialRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } if len(prompter.calls) == 0 { diff --git a/internal/cmd/me/me.go b/internal/cmd/me/me.go index f4673ae..478a066 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, refErr := keychain.ResolveEffectiveCredentialRef(); refErr == 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/me/me_test.go b/internal/cmd/me/me_test.go index d315974..7ebf719 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,41 @@ func TestRunStaleRecordedScopesTriggersReauthMessage(t *testing.T) { } } +func TestRunStaleScopesUseSelectedProfile(t *testing.T) { + // Not Parallel: mutates env, the selected-ref override, and ClientFactory. + withConfigDir(t) + ref := "google-readonly/personal" + t.Setenv(keychain.CredentialRefEnvVar(), "") + keychain.SetCredentialRefOverride(ref, true) + t.Cleanup(func() { keychain.SetCredentialRefOverride("", false) }) + if err := config.SaveConfig(&config.Config{ + CredentialRef: config.DefaultCredentialRef, + GrantedScopes: config.Scopes(), + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + ref: {GrantedScopes: []string{"https://www.googleapis.com/auth/gmail.modify"}}, + }, + }); err != nil { + t.Fatal(err) + } + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + t.Fatal("client should not be called when 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 selected profile, got %v", err) + } + if !strings.Contains(errOut.String(), "gro init") { + t.Fatalf("expected reauth guidance for selected profile, got %q", errOut.String()) + } + if got := grantedScopes(); len(got) != 1 || got[0] != "https://www.googleapis.com/auth/gmail.modify" { + t.Fatalf("extended scopes = %v, want the selected profile's recorded scope", got) + } +} + func TestRunMissingConfigDoesNotShortCircuit(t *testing.T) { // Not Parallel: mutates env + ClientFactory. withConfigDir(t) diff --git a/internal/cmd/profiles/profiles.go b/internal/cmd/profiles/profiles.go index 3c01d9b..2a28ba6 100644 --- a/internal/cmd/profiles/profiles.go +++ b/internal/cmd/profiles/profiles.go @@ -364,6 +364,11 @@ func runRename(oldProfile, newProfile string) error { if err != nil { return err } + if oldRef != newRef { + if _, exists := cfg.ProfileOAuth[newRef]; exists { + return fmt.Errorf("profile OAuth config for %s already exists", newRef) + } + } st, err := OpenRefStore(oldRef) if err != nil { return err @@ -388,14 +393,23 @@ func runRename(oldProfile, newProfile string) error { if activeChanged { cfg.CredentialRef = newRef cfg.SetCredentialRefSource(config.RefSourceConfig) + } + profileOAuth, profileOAuthMoved := cfg.ProfileOAuth[oldRef] + if profileOAuthMoved { + // Keep the source association until the source token has been deleted. + // If that deletion fails, both stored tokens still resolve to their + // original client configuration. + cfg.ProfileOAuth[newRef] = profileOAuth + } + if activeChanged || profileOAuthMoved { if err := renameSaveConfig(cfg); err != nil { // 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: %w)", oldRef, err, rollbackErr) + return fmt.Errorf("saving profile configuration after copying credentials failed; source was retained and copied destination may remain: %w (rollback failed: %w)", err, rollbackErr) } - return fmt.Errorf("saving active profile %s failed after copying credentials; source was retained and copied destination was removed: %w", oldRef, err) + return fmt.Errorf("saving profile configuration after copying credentials failed; source was retained and copied destination was removed: %w", err) } } @@ -404,6 +418,15 @@ func runRename(oldProfile, newProfile string) error { 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) } + if profileOAuthMoved { + delete(cfg.ProfileOAuth, oldRef) + if err := renameSaveConfig(cfg); err != nil { + // Credentials have already moved. A stale source mapping is safe for + // the current rename and can be cleaned up later; never roll back the + // destination or recreate a token just to remove it. + fmt.Fprintf(os.Stderr, "warning: credentials renamed from %s to %s but the old profile OAuth association could not be removed: %v\n", oldRef, newRef, err) + } + } // Identity data is disposable, but preserving its verification timestamp // makes the rename transparent to `profiles list`. diff --git a/internal/cmd/profiles/profiles_test.go b/internal/cmd/profiles/profiles_test.go index 7bcf025..7c12942 100644 --- a/internal/cmd/profiles/profiles_test.go +++ b/internal/cmd/profiles/profiles_test.go @@ -442,6 +442,76 @@ func TestRunRename_NonActivePreservesSavedConfig(t *testing.T) { assertToken(t, "new", "A-old") } +func TestRunRename_MovesProfileOAuthAssociationWithoutChangingActiveRef(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + clientPath := filepath.Join(t.TempDir(), "oauth-client-profile-test.json") + originalScopes := []string{"scope:mail", "scope:profile"} + if err := config.SaveConfig(&config.Config{ + CredentialRef: "google-readonly/current", + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + "google-readonly/old": {OAuthClientPath: clientPath, GrantedScopes: originalScopes}, + }, + }); 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 != "google-readonly/current" { + t.Fatalf("active credential_ref = %q, want unchanged current profile", got.CredentialRef) + } + if _, exists := got.ProfileOAuth["google-readonly/old"]; exists { + t.Fatal("old profile OAuth association remains after rename") + } + profile, exists := got.ProfileOAuth["google-readonly/new"] + if !exists { + t.Fatal("new profile has no OAuth association") + } + if profile.OAuthClientPath != clientPath { + t.Fatalf("client path after rename = %q, want unchanged %q", profile.OAuthClientPath, clientPath) + } + if len(profile.GrantedScopes) != len(originalScopes) || profile.GrantedScopes[0] != originalScopes[0] || profile.GrantedScopes[1] != originalScopes[1] { + t.Fatalf("scopes after rename = %v, want %v", profile.GrantedScopes, originalScopes) + } + assertToken(t, "new", "A-old") + assertNoToken(t, "old") +} + +func TestRunRename_ProfileOAuthDestinationCollisionRetainsSource(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + oldPath := filepath.Join(t.TempDir(), "old-client.json") + newPath := filepath.Join(t.TempDir(), "new-client.json") + if err := config.SaveConfig(&config.Config{ + CredentialRef: "google-readonly/default", + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + "google-readonly/old": {OAuthClientPath: oldPath}, + "google-readonly/new": {OAuthClientPath: newPath}, + }, + }); err != nil { + t.Fatal(err) + } + err := runRenameQuiet(t, "old", "new") + if err == nil || !strings.Contains(err.Error(), "profile OAuth config") { + t.Fatalf("rename with config collision = %v, want profile OAuth config error", err) + } + got, err := config.LoadConfigForRuntime() + if err != nil { + t.Fatal(err) + } + if got.ProfileOAuth["google-readonly/old"].OAuthClientPath != oldPath || got.ProfileOAuth["google-readonly/new"].OAuthClientPath != newPath { + t.Fatalf("profile associations changed after collision: %#v", got.ProfileOAuth) + } + assertToken(t, "old", "A-old") + assertNoToken(t, "new") +} + func TestRunRename_CollisionRetainsSourceAndDestination(t *testing.T) { credtest.Setup(t) seedToken(t, "old") @@ -582,7 +652,14 @@ func TestRunRename_RetryAfterTransientConfigFailure(t *testing.T) { func TestRunRename_DeleteFailureRetainsBothBundles(t *testing.T) { credtest.Setup(t) seedToken(t, "old") - if err := config.SaveConfig(&config.Config{CredentialRef: "google-readonly/old"}); err != nil { + clientPath := filepath.Join(t.TempDir(), "old-client.json") + scopes := []string{"scope:mail", "scope:profile"} + if err := config.SaveConfig(&config.Config{ + CredentialRef: "google-readonly/old", + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + "google-readonly/old": {OAuthClientPath: clientPath, GrantedScopes: scopes}, + }, + }); err != nil { t.Fatal(err) } original := renameDelete @@ -602,6 +679,62 @@ func TestRunRename_DeleteFailureRetainsBothBundles(t *testing.T) { if cfg.CredentialRef != "google-readonly/new" { t.Fatalf("credential_ref after delete failure = %q, want new", cfg.CredentialRef) } + for _, ref := range []string{"google-readonly/old", "google-readonly/new"} { + if got := cfg.OAuthClientPathForRef(ref); got != clientPath { + t.Errorf("OAuth client path for %s = %q, want %q", ref, got, clientPath) + } + if got := cfg.GrantedScopesForRef(ref); len(got) != len(scopes) || got[0] != scopes[0] || got[1] != scopes[1] { + t.Errorf("granted scopes for %s = %v, want %v", ref, got, scopes) + } + } + assertToken(t, "old", "A-old") + assertToken(t, "new", "A-old") +} + +func TestRunRename_ProfileOAuthCleanupFailureWarnsAfterTokenMove(t *testing.T) { + credtest.Setup(t) + seedToken(t, "old") + clientPath := filepath.Join(t.TempDir(), "old-client.json") + if err := config.SaveConfig(&config.Config{ + CredentialRef: "google-readonly/old", + ProfileOAuth: map[string]config.ProfileOAuthConfig{ + "google-readonly/old": {OAuthClientPath: clientPath}, + }, + }); err != nil { + t.Fatal(err) + } + original := renameSaveConfig + attempts := 0 + renameSaveConfig = func(cfg *config.Config) error { + attempts++ + if attempts == 2 { + return errors.New("cleanup config unavailable") + } + return original(cfg) + } + t.Cleanup(func() { renameSaveConfig = original }) + + var runErr error + capture(t, func() { + stderr := captureStderr(t, func() { runErr = runRename("old", "new") }) + if !strings.Contains(stderr, "old profile OAuth association could not be removed") { + t.Errorf("stderr = %q, want stale mapping warning", stderr) + } + }) + if runErr != nil { + t.Fatalf("runRename after OAuth cleanup failure: %v", runErr) + } + assertNoToken(t, "old") + assertToken(t, "new", "A-old") + cfg, err := config.LoadConfigForRuntime() + if err != nil { + t.Fatal(err) + } + for _, ref := range []string{"google-readonly/old", "google-readonly/new"} { + if got := cfg.OAuthClientPathForRef(ref); got != clientPath { + t.Errorf("OAuth client path for %s = %q, want %q", ref, got, clientPath) + } + } } func TestRunRename_IgnoresInvocationSelectorForSource(t *testing.T) { diff --git a/internal/config/config.go b/internal/config/config.go index bd2d62c..b403f49 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -74,6 +74,10 @@ type Config struct { // GrantedScopes is preserved: detects when a token's scopes drift from // what init granted. Not a secret. GrantedScopes []string `yaml:"granted_scopes,omitempty" json:"granted_scopes,omitempty"` + // ProfileOAuth stores client/scopes owned by a specific credential_ref. + // Missing entries continue to use the legacy shared OAuthClientPath and + // GrantedScopes fields above. + ProfileOAuth map[string]ProfileOAuthConfig `yaml:"profile_oauth,omitempty" json:"profile_oauth,omitempty"` // Keyring carries the optional §1.4 explicit file-backend opt-in. Keyring KeyringConfig `yaml:"keyring,omitempty" json:"-"` @@ -85,6 +89,51 @@ type Config struct { credentialRefSource RefSource } +// ProfileOAuthConfig binds a profile to its own OAuth desktop client JSON and +// records the scopes that profile granted. It contains no access token. +type ProfileOAuthConfig struct { + OAuthClientPath string `yaml:"oauth_client_path,omitempty" json:"oauth_client_path,omitempty"` + GrantedScopes []string `yaml:"granted_scopes,omitempty" json:"granted_scopes,omitempty"` +} + +// OAuthClientPathForRef returns the selected profile's client JSON path, or +// the legacy shared path when that ref has no profile-specific association. +func (c *Config) OAuthClientPathForRef(ref string) string { + if c == nil { + return "" + } + if profile, ok := c.ProfileOAuth[ref]; ok && profile.OAuthClientPath != "" { + return ExpandPath(profile.OAuthClientPath) + } + return ExpandPath(c.OAuthClientPath) +} + +// GrantedScopesForRef returns a copy of the scopes recorded for ref. A +// profile-specific entry is authoritative even when its list is empty; this +// avoids treating another profile's consent as this profile's. +func (c *Config) GrantedScopesForRef(ref string) []string { + if c == nil { + return nil + } + if profile, ok := c.ProfileOAuth[ref]; ok { + return append([]string(nil), profile.GrantedScopes...) + } + return append([]string(nil), c.GrantedScopes...) +} + +// SetProfileOAuth updates one profile's OAuth-client association and recorded +// scopes without changing the legacy shared-client fallback. +func (c *Config) SetProfileOAuth(ref string, profile ProfileOAuthConfig) { + if c.ProfileOAuth == nil { + c.ProfileOAuth = make(map[string]ProfileOAuthConfig) + } + if profile.OAuthClientPath != "" { + profile.OAuthClientPath = ExpandPath(profile.OAuthClientPath) + } + profile.GrantedScopes = append([]string(nil), profile.GrantedScopes...) + c.ProfileOAuth[ref] = profile +} + // 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. @@ -392,6 +441,13 @@ func (c *Config) applyDefaults() { } else { c.OAuthClientPath = ExpandPath(c.OAuthClientPath) } + for ref, profile := range c.ProfileOAuth { + if profile.OAuthClientPath != "" { + profile.OAuthClientPath = ExpandPath(profile.OAuthClientPath) + } + profile.GrantedScopes = append([]string(nil), profile.GrantedScopes...) + c.ProfileOAuth[ref] = profile + } } // SaveConfig writes config.yml at 0600 under a 0700 directory using an atomic @@ -409,6 +465,16 @@ func SaveConfig(cfg *Config) error { // caller's *Config (a caller inspecting OAuthClientPath after SaveConfig // would otherwise observe an unexpectedly rewritten value). out := *cfg + if cfg.ProfileOAuth != nil { + out.ProfileOAuth = make(map[string]ProfileOAuthConfig, len(cfg.ProfileOAuth)) + for ref, profile := range cfg.ProfileOAuth { + if profile.OAuthClientPath != "" { + profile.OAuthClientPath = ExpandPath(profile.OAuthClientPath) + } + profile.GrantedScopes = append([]string(nil), profile.GrantedScopes...) + out.ProfileOAuth[ref] = profile + } + } if out.OAuthClientPath != "" { out.OAuthClientPath = ExpandPath(out.OAuthClientPath) } @@ -442,3 +508,28 @@ func SaveConfig(cfg *Config) error { } return nil } + +// NewProfileOAuthClientPath reserves a unique managed location for an +// explicitly imported profile client. The caller writes the validated JSON +// there and removes the file if its config association cannot be saved. +func NewProfileOAuthClientPath() (string, error) { + dir, err := GetConfigDir() + if err != nil { + return "", err + } + f, err := os.CreateTemp(dir, "oauth-client-profile-*.json") + if err != nil { + return "", fmt.Errorf("creating profile OAuth client path: %w", err) + } + path := f.Name() + if err := f.Chmod(TokenPerm); err != nil { + _ = f.Close() + _ = os.Remove(path) + return "", fmt.Errorf("setting profile OAuth client permissions: %w", err) + } + if err := f.Close(); err != nil { + _ = os.Remove(path) + return "", fmt.Errorf("closing profile OAuth client file: %w", err) + } + return path, nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 02b0358..004b5a0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -312,6 +312,71 @@ func TestSaveConfig(t *testing.T) { }) } +func TestProfileOAuthResolversAndRoundTrip(t *testing.T) { + hermeticConfig(t) + home, err := os.UserHomeDir() + if err != nil { + t.Fatal(err) + } + ref := "google-readonly/personal" + sharedPath := filepath.Join(home, "shared.json") + profilePath := filepath.Join(home, "personal.json") + sharedScopes := []string{"scope:shared"} + profileScopes := []string{"scope:personal"} + cfg := &Config{ + OAuthClientPath: sharedPath, + GrantedScopes: sharedScopes, + ProfileOAuth: map[string]ProfileOAuthConfig{ + ref: {OAuthClientPath: "~/personal.json", GrantedScopes: profileScopes}, + }, + } + + if got := cfg.OAuthClientPathForRef(ref); got != profilePath { + t.Fatalf("profile OAuth path = %q, want %q", got, profilePath) + } + if got := cfg.OAuthClientPathForRef("google-readonly/work"); got != sharedPath { + t.Fatalf("fallback OAuth path = %q, want %q", got, sharedPath) + } + gotScopes := cfg.GrantedScopesForRef(ref) + if len(gotScopes) != 1 || gotScopes[0] != "scope:personal" { + t.Fatalf("profile scopes = %v, want personal scope", gotScopes) + } + gotScopes[0] = "mutated" + if cfg.ProfileOAuth[ref].GrantedScopes[0] != "scope:personal" { + t.Fatal("GrantedScopesForRef must return a copy") + } + if got := cfg.GrantedScopesForRef("google-readonly/work"); len(got) != 1 || got[0] != "scope:shared" { + t.Fatalf("fallback scopes = %v, want shared scope", got) + } + + if err := SaveConfig(cfg); err != nil { + t.Fatalf("SaveConfig: %v", err) + } + loaded, err := LoadConfig() + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + if got := loaded.OAuthClientPathForRef(ref); got != profilePath { + t.Fatalf("round-trip profile OAuth path = %q, want %q", got, profilePath) + } + if got := loaded.GrantedScopesForRef(ref); len(got) != 1 || got[0] != "scope:personal" { + t.Fatalf("round-trip profile scopes = %v, want personal scope", got) + } +} + +func TestProfileOAuthEmptyScopesAreAuthoritative(t *testing.T) { + t.Parallel() + cfg := &Config{ + GrantedScopes: []string{"scope:shared"}, + ProfileOAuth: map[string]ProfileOAuthConfig{ + "google-readonly/personal": {OAuthClientPath: "/tmp/personal.json"}, + }, + } + if got := cfg.GrantedScopesForRef("google-readonly/personal"); len(got) != 0 { + t.Fatalf("profile with no recorded scopes inherited shared scopes: %v", got) + } +} + func TestCacheDirResolvers(t *testing.T) { t.Run("GetCacheDir is under os.UserCacheDir()/DirName, not the config tree", func(t *testing.T) { hermeticConfig(t) diff --git a/internal/config/relocate.go b/internal/config/relocate.go index 48d2a35..7f1e074 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -9,6 +9,7 @@ import ( "path/filepath" "reflect" "sort" + "strings" "sync" "gopkg.in/yaml.v3" @@ -212,9 +213,36 @@ func configsMaterialEqual(a, b Config, oldDir, newDir string) bool { if !oauthClientPathEquiv(a.OAuthClientPath, b.OAuthClientPath, oldDir, newDir) { return false } + if len(a.ProfileOAuth) != len(b.ProfileOAuth) { + return false + } + for ref, aProfile := range a.ProfileOAuth { + bProfile, ok := b.ProfileOAuth[ref] + if !ok || !slicesEqualSorted(aProfile.GrantedScopes, bProfile.GrantedScopes) { + return false + } + if !profileOAuthClientPathEquiv(aProfile.OAuthClientPath, bProfile.OAuthClientPath, oldDir, newDir) { + return false + } + } return true } +// profileOAuthClientPathEquiv also recognizes managed per-profile imports +// copied with their config directory during relocation. User-selected paths +// outside those directories still require an exact match. +func profileOAuthClientPathEquiv(aPath, bPath, aDir, bDir string) bool { + if oauthClientPathEquiv(aPath, bPath, aDir, bDir) { + return true + } + aPath = ExpandPath(aPath) + bPath = ExpandPath(bPath) + base := filepath.Base(aPath) + return strings.HasPrefix(base, "oauth-client-profile-") && + base == filepath.Base(bPath) && + filepath.Dir(aPath) == aDir && filepath.Dir(bPath) == bDir +} + // 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 diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go index 5e37535..e8b764d 100644 --- a/internal/config/relocate_test.go +++ b/internal/config/relocate_test.go @@ -21,6 +21,47 @@ func reloctest(t *testing.T) (oldDir, newDir string) { return oldDir, newDir } +func TestConfigsMaterialEqual_ProfileOAuthAssociations(t *testing.T) { + oldDir, newDir := reloctest(t) + name := "oauth-client-profile-a1b2.json" + oldCfg := Config{ + CredentialRef: "google-readonly/default", + ProfileOAuth: map[string]ProfileOAuthConfig{ + "google-readonly/personal": { + OAuthClientPath: filepath.Join(oldDir, name), + GrantedScopes: []string{"scope:a", "scope:b"}, + }, + }, + } + newCfg := Config{ + CredentialRef: "google-readonly/default", + ProfileOAuth: map[string]ProfileOAuthConfig{ + "google-readonly/personal": { + OAuthClientPath: filepath.Join(newDir, name), + GrantedScopes: []string{"scope:b", "scope:a"}, + }, + }, + } + if !configsMaterialEqual(oldCfg, newCfg, oldDir, newDir) { + t.Fatal("matching profile associations under relocated dirs should be equivalent") + } + + newCfg.ProfileOAuth["google-readonly/personal"] = ProfileOAuthConfig{ + OAuthClientPath: filepath.Join(newDir, name), + GrantedScopes: []string{"scope:a"}, + } + if configsMaterialEqual(oldCfg, newCfg, oldDir, newDir) { + t.Fatal("different profile grants must be a relocation conflict") + } + newCfg.ProfileOAuth["google-readonly/personal"] = ProfileOAuthConfig{ + OAuthClientPath: filepath.Join(newDir, "oauth-client-profile-other.json"), + GrantedScopes: []string{"scope:a", "scope:b"}, + } + if configsMaterialEqual(oldCfg, newCfg, oldDir, newDir) { + t.Fatal("different profile client paths must be a relocation conflict") + } +} + // detectAt exercises the pure-function core that takes an injected newDir, // so the four cases are testable on Linux even though Linux's real // os.UserConfigDir collapses to old==new. diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 9b1cd24..bc1a382 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -134,6 +134,22 @@ func effectiveRef(configRef string) (ref string, source config.RefSource, overri return configRef, "", false } +// ResolveEffectiveCredentialRef applies the same flag > env > config +// precedence used by Open, without opening a credential backend. Read-only +// metadata checks use it to select per-profile config before constructing an +// API client. +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 `gro set-credential --ref` and the refresh // persister. An empty ref falls back to the configured/default ref. From a14f533033a29a86dc9aa1325a159bb21ac25ba5 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 20 Sep 2026 07:34:10 -0400 Subject: [PATCH 7/7] fix: keep imported OAuth clients profile-scoped --- internal/cmd/init/init.go | 22 ++---- internal/cmd/init/init_test.go | 124 +++++++++++++++++++++++++----- internal/cmd/init/sibling_test.go | 22 +++++- 3 files changed, 131 insertions(+), 37 deletions(-) diff --git a/internal/cmd/init/init.go b/internal/cmd/init/init.go index 63e914c..7591c30 100644 --- a/internal/cmd/init/init.go +++ b/internal/cmd/init/init.go @@ -751,8 +751,8 @@ 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. +// ensureCredentials makes sure an OAuth client exists for targetRef, importing +// user-supplied JSON into a profile-managed file when needed. func ensureCredentials(d initDeps, opts *initOptions, credPath, targetRef string) error { // --credentials-file flag wins. if opts.credentialsFile != "" { @@ -774,7 +774,7 @@ func ensureCredentials(d initDeps, opts *initOptions, credPath, targetRef string if d.DiscoverSiblingClientJSON != nil { 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) + return importProfileFromFile(d, srcPath, targetRef) } } @@ -853,11 +853,10 @@ func ensureCredentials(d initDeps, opts *initOptions, credPath, targetRef string return fmt.Errorf("unknown choice: %s", choice) } - if err := writeCredentials(d, credPath, blob); err != nil { + if err := importProfileJSON(d, blob, targetRef); err != nil { d.View.Error("%v", err) continue } - d.View.Success("Credentials saved to %s", credPath) return nil } return errors.New("could not obtain valid credentials.json after 3 attempts") @@ -871,6 +870,10 @@ func importProfileFromFile(d initDeps, srcPath, targetRef string) error { if err != nil { return fmt.Errorf("reading %s: %w", srcPath, err) } + return importProfileJSON(d, blob, targetRef) +} + +func importProfileJSON(d initDeps, blob []byte, targetRef string) error { imported, err := google.ConfigFromJSON(blob, config.Scopes()...) if err != nil { return fmt.Errorf("invalid OAuth client JSON: %w", err) @@ -949,15 +952,6 @@ func removeProfileOAuthClientFile(d initDeps, path string) error { return os.Remove(path) } -// importFromFile reads, validates, and writes credentials.json from a path. -func importFromFile(d initDeps, srcPath, dstPath string) error { - blob, err := d.ReadFile(srcPath) - if err != nil { - return fmt.Errorf("reading %s: %w", srcPath, err) - } - return writeCredentials(d, dstPath, blob) -} - // 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 5378f73..1ca3f96 100644 --- a/internal/cmd/init/init_test.go +++ b/internal/cmd/init/init_test.go @@ -409,29 +409,117 @@ func TestEnsureCredentialsClipboardWizard(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) + const targetRef = "google-readwrite/personal" + targetPath, sharedPath := separateProfileCredentialPath(t, &d, targetRef) + profilePath, _ := d.NewProfileOAuthClientPath() + const sharedClient = "existing-shared-client" + fs.files[sharedPath] = []byte(sharedClient) d.ClipboardSupported = func() bool { return true } d.ClipboardReadAll = func() (string, error) { return validOAuthJSON, nil } - dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) d.Prompter = &stubPrompter{credChoice: "clipboard"} - if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { + if err := ensureCredentials(d, &initOptions{}, targetPath, targetRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } - if _, err := fs.ReadFile(dst); err != nil { - t.Fatalf("dst not written: %v", err) - } + assertProfileOAuthImport(t, fs, d, targetRef, targetPath, sharedPath, profilePath, sharedClient) } func TestEnsureCredentialsPasteWizard(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) - dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) + const targetRef = "google-readwrite/personal" + targetPath, sharedPath := separateProfileCredentialPath(t, &d, targetRef) + profilePath, _ := d.NewProfileOAuthClientPath() + const sharedClient = "existing-shared-client" + fs.files[sharedPath] = []byte(sharedClient) d.Prompter = &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON} - if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { + if err := ensureCredentials(d, &initOptions{}, targetPath, targetRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } - if _, err := fs.ReadFile(dst); err != nil { - t.Fatalf("dst not written: %v", err) + assertProfileOAuthImport(t, fs, d, targetRef, targetPath, sharedPath, profilePath, sharedClient) +} + +func separateProfileCredentialPath(t *testing.T, d *initDeps, targetRef string) (targetPath, sharedPath string) { + t.Helper() + var err error + sharedPath, err = d.GetCredentialsPath(config.DefaultCredentialRef) + if err != nil { + t.Fatal(err) + } + targetPath = filepath.Join(filepath.Dir(sharedPath), "credentials-personal.json") + getCredentialsPath := d.GetCredentialsPath + d.GetCredentialsPath = func(ref string) (string, error) { + if ref == targetRef { + return targetPath, nil + } + return getCredentialsPath(ref) + } + return targetPath, sharedPath +} + +func assertProfileOAuthImport(t *testing.T, fs *fakeFS, d initDeps, targetRef, targetPath, sharedPath, profilePath, sharedClient string) { + t.Helper() + got, err := fs.ReadFile(profilePath) + if err != nil { + t.Fatalf("profile OAuth client not written: %v", err) + } + if string(got) != validOAuthJSON { + t.Errorf("profile OAuth client content mismatch") + } + if got := string(fs.files[sharedPath]); got != sharedClient { + t.Fatalf("shared OAuth client changed to %q", got) + } + if _, err := fs.ReadFile(targetPath); !os.IsNotExist(err) { + t.Fatalf("selected profile credential path should not be used for imported client JSON, got err=%v", err) + } + cfg, err := d.LoadConfig() + if err != nil { + t.Fatal(err) + } + if got := cfg.ProfileOAuth[targetRef].OAuthClientPath; got != profilePath { + t.Fatalf("profile OAuth association = %q, want %q", got, profilePath) + } +} + +func TestEnsureCredentialsPasteWizardRejectsDifferentClientWhenTokenExists(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + const targetRef = "google-readwrite/personal" + targetPath, sharedPath := separateProfileCredentialPath(t, &d, targetRef) + profilePath, _ := d.NewProfileOAuthClientPath() + const sharedClient = "existing-shared-client" + const oldProfileClient = "/existing/profile-client.json" + fs.files[sharedPath] = []byte(sharedClient) + cfg := &config.Config{CredentialRef: targetRef} + cfg.SetProfileOAuth(targetRef, config.ProfileOAuthConfig{OAuthClientPath: oldProfileClient}) + d.LoadConfig = func() (*config.Config, error) { return cfg, nil } + d.HasStoredToken = func() bool { return true } + d.GetOAuthConfig = func(ref string) (*oauth2.Config, error) { + if ref != targetRef { + t.Fatalf("OAuth config requested for %q, want %q", ref, targetRef) + } + return &oauth2.Config{ClientID: "existing.apps.googleusercontent.com"}, nil + } + var stdout, stderr bytes.Buffer + d.View = view.NewWithWriters(&stdout, &stderr) + d.Prompter = &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON} + + err := ensureCredentials(d, &initOptions{}, targetPath, targetRef) + if err == nil { + t.Fatal("expected the wizard to refuse binding a different OAuth client to the stored token") + } + if !strings.Contains(stdout.String()+stderr.String(), "different client ID") { + t.Fatalf("wizard output does not explain the client-ID guard: %q %q", stdout.String(), stderr.String()) + } + if got := cfg.OAuthClientPathForRef(targetRef); got != oldProfileClient { + t.Fatalf("profile OAuth path changed to %q, want %q", got, oldProfileClient) + } + if _, err := fs.ReadFile(profilePath); err == nil { + t.Fatal("staged profile OAuth JSON remained after refusing import") + } + if got := string(fs.files[sharedPath]); got != sharedClient { + t.Fatalf("shared OAuth client changed to %q", got) } } @@ -445,20 +533,18 @@ func TestEnsureCredentialsFileWizard(t *testing.T) { if err := os.WriteFile(src, []byte(validOAuthJSON), 0644); err != nil { t.Fatal(err) } - dst, _ := d.GetCredentialsPath(config.DefaultCredentialRef) + const targetRef = "google-readwrite/personal" + targetPath, sharedPath := separateProfileCredentialPath(t, &d, targetRef) + profilePath, _ := d.NewProfileOAuthClientPath() + const sharedClient = "existing-shared-client" + fs.files[sharedPath] = []byte(sharedClient) d.Prompter = &stubPrompter{credChoice: "file", filePath: src} - if err := ensureCredentials(d, &initOptions{}, dst, config.DefaultCredentialRef); err != nil { + if err := ensureCredentials(d, &initOptions{}, targetPath, targetRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } - got, err := fs.ReadFile(dst) - if err != nil { - t.Fatalf("dst not written: %v", err) - } - if string(got) != validOAuthJSON { - t.Errorf("dst content mismatch") - } - if perm := fs.perms[dst]; perm != 0600 { + assertProfileOAuthImport(t, fs, d, targetRef, targetPath, sharedPath, profilePath, sharedClient) + if perm := fs.perms[profilePath]; perm != 0600 { t.Errorf("expected perms 0600, got %o", perm) } } diff --git a/internal/cmd/init/sibling_test.go b/internal/cmd/init/sibling_test.go index 4794049..e39b06e 100644 --- a/internal/cmd/init/sibling_test.go +++ b/internal/cmd/init/sibling_test.go @@ -14,19 +14,33 @@ func TestEnsureCredentials_ReusesSiblingOAuthClient(t *testing.T) { fs := newFakeFS() d := baseDeps(t, fs) - credPath := filepath.Join(t.TempDir(), "oauth_client.json") + const targetRef = "google-readwrite/personal" + credPath, sharedPath := separateProfileCredentialPath(t, &d, targetRef) + profilePath, _ := d.NewProfileOAuthClientPath() siblingPath := filepath.Join(t.TempDir(), "sibling-oauth_client.json") + const legacyClient = "existing-shared-client" + fs.files[sharedPath] = []byte(legacyClient) fs.files[siblingPath] = []byte(validOAuthJSON) prompter := &stubPrompter{} d.Prompter = prompter d.DiscoverSiblingClientJSON = func() (string, string, bool) { return siblingPath, "google-readonly", true } - if err := ensureCredentials(d, &initOptions{}, credPath, config.DefaultCredentialRef); err != nil { + if err := ensureCredentials(d, &initOptions{}, credPath, targetRef); err != nil { t.Fatalf("ensureCredentials: %v", err) } - if _, ok := fs.files[credPath]; !ok { - t.Fatal("expected the sibling OAuth client JSON to be written to credPath") + if got, ok := fs.files[profilePath]; !ok || string(got) != validOAuthJSON { + t.Fatal("expected the sibling OAuth client JSON to be copied to the selected profile") + } + if got := string(fs.files[sharedPath]); got != legacyClient { + t.Fatalf("profile-scoped sibling import changed the legacy shared path to %q", got) + } + cfg, err := d.LoadConfig() + if err != nil { + t.Fatal(err) + } + if got := cfg.ProfileOAuth[targetRef].OAuthClientPath; got != profilePath { + t.Fatalf("sibling OAuth client association = %q, want %q", got, profilePath) } if len(prompter.calls) != 0 { t.Errorf("the paste wizard must not run when a sibling client is reused; calls=%v", prompter.calls)