diff --git a/docs/development.md b/docs/development.md index 3e9ea45..48b3931 100644 --- a/docs/development.md +++ b/docs/development.md @@ -63,7 +63,7 @@ The required checks on `main` are `build`, `tidy`, `test`, `lint`, `static-relea ## Releases -There is one release stream for both binaries. `version.txt` contains the major/minor line (`1.2`), and automatic releases create `v1.2.N` tags. A squash merge to `main` whose final commit begins with `feat:` or `fix:` triggers the automatic release decision when it changes Go source, `go.mod`, `go.sum`, `version.txt`, or `.goreleaser.yaml`. GoReleaser configuration changes therefore ship too; documentation, test, CI, and chore-only merges do not cut a release. +There is one release stream for both binaries. `version.txt` contains the major/minor line (`2.0`), and automatic releases create `v2.0.N` tags. A squash merge to `main` whose final commit begins with `feat:` or `fix:` triggers the automatic release decision when it changes Go source, `go.mod`, `go.sum`, `version.txt`, or `.goreleaser.yaml`. GoReleaser configuration changes therefore ship too; documentation, test, CI, and chore-only merges do not cut a release. Each tag publishes both binaries and their platform archives through the shared release automation. Homebrew, Chocolatey, WinGet, and Linux package publication fan out from that release. Follow the [release](https://github.com/open-cli-collective/cli-common/blob/main/docs/release.md) and [distribution](https://github.com/open-cli-collective/cli-common/blob/main/docs/distribution.md) standards rather than duplicating workflow policy here. @@ -81,8 +81,7 @@ and asserts it selects the Keychain backend, which catches a static or mis-tagge is published. `version.txt` holds only `MAJOR.MINOR`; the `major_minor_run_patch` scheme appends the workflow -run number, so tags are `v1.2.N` and never collide or need a bump commit. The stream starts at -`1.2` because `gro` had already released `1.1.x`; continuing its line keeps upgrades monotonic for -existing installs. +run number, so tags are `v2.0.N` and never collide or need a bump commit. The stream starts at +`2.0` because replacing the public credential selector is a breaking CLI change. Use a focused branch, run `make check`, and open a pull request. Keep the pull-request title in conventional-commit form because squash merge makes that title the commit on `main` and therefore the release signal. diff --git a/internal/app/gro/backend_wire_test.go b/internal/app/gro/backend_wire_test.go index f359fb5..0b31c19 100644 --- a/internal/app/gro/backend_wire_test.go +++ b/internal/app/gro/backend_wire_test.go @@ -10,6 +10,7 @@ import ( cccredstore "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/google-cli/internal/keychain" + "github.com/open-cli-collective/google-cli/internal/rootutil" ) const serviceName = "google-readonly" @@ -26,16 +27,28 @@ func resetState(t *testing.T) { t.Helper() keychain.SetBackendFlagOverride("", false) keychain.SetCredentialRefOverride("", false) + resetRootFlag(t, cccredstore.BackendFlagName) + resetRootFlag(t, rootutil.ProfileFlagName) // rootCmd.SetArgs mutates package-level state; if a test panics before // the next test calls SetArgs, a stale slice could bleed in (notably // under `go test -shuffle=on`). Clear it on cleanup. t.Cleanup(func() { keychain.SetBackendFlagOverride("", false) keychain.SetCredentialRefOverride("", false) + resetRootFlag(t, cccredstore.BackendFlagName) + resetRootFlag(t, rootutil.ProfileFlagName) rootCmd.SetArgs(nil) }) } +func resetRootFlag(t *testing.T, name string) { + t.Helper() + if f := rootCmd.PersistentFlags().Lookup(name); f != nil { + _ = f.Value.Set(f.DefValue) + f.Changed = false + } +} + // newProbeCmd returns a no-op subcommand used to exercise the root's // PersistentPreRunE through a real Execute() call. func newProbeCmd(name string) *cobra.Command { diff --git a/internal/app/gro/credref_wire_test.go b/internal/app/gro/credref_wire_test.go index 466d451..3858a35 100644 --- a/internal/app/gro/credref_wire_test.go +++ b/internal/app/gro/credref_wire_test.go @@ -10,57 +10,56 @@ import ( "github.com/open-cli-collective/google-cli/internal/rootutil" ) -// TestWireCredentialRefSelection_FlagSet proves a --ref on a real command -// path is recorded in the override the keychain.open resolver reads. func TestWireCredentialRefSelection_FlagSet(t *testing.T) { resetState(t) t.Setenv(keychain.CredentialRefEnvVar(), "") - probe := newProbeCmd("probe-ref-flagset") + probe := newProbeCmd("probe-profile-flagset") rootCmd.AddCommand(probe) defer removeChild(t, probe) - rootCmd.SetArgs([]string{"probe-ref-flagset", "--ref", "google-readonly/acct-a"}) + rootCmd.SetArgs([]string{"probe-profile-flagset", "--profile", "acct-a"}) if err := rootCmd.Execute(); err != nil { t.Fatalf("Execute: %v", err) } v, set := keychain.GetCredentialRefOverride() - if !set { - t.Fatalf("override flagSet = false, want true") - } - if v != "google-readonly/acct-a" { - t.Errorf("override value = %q, want %q", v, "google-readonly/acct-a") + if !set || v != "google-readonly/acct-a" { + t.Errorf("override = (%q, %v), want (google-readonly/acct-a, true)", v, set) } } -// TestWireCredentialRefSelection_FlagInvalid asserts a malformed --ref fails -// up front with a clear "--ref" error, before any keyring work. -func TestWireCredentialRefSelection_FlagInvalid(t *testing.T) { +func TestWireCredentialRefSelection_InvalidStopsBeforeLeaf(t *testing.T) { resetState(t) - - probe := newProbeCmd("probe-ref-invalid") + called := false + probe := &cobra.Command{ + Use: "probe-profile-sentinel", + RunE: func(*cobra.Command, []string) error { + called = true + return nil + }, + } rootCmd.AddCommand(probe) defer removeChild(t, probe) - rootCmd.SetArgs([]string{"probe-ref-invalid", "--ref", "no-slash"}) + rootCmd.SetArgs([]string{"probe-profile-sentinel", "--profile", "bad.profile"}) err := rootCmd.Execute() - if err == nil { - t.Fatal("expected error, got nil") + if err == nil || !strings.Contains(err.Error(), "--"+rootutil.ProfileFlagName) { + t.Fatalf("expected invalid --profile error, got %v", err) } - if !strings.Contains(err.Error(), "--"+rootutil.CredentialRefFlagName) { - t.Errorf("error should mention --%s: %v", rootutil.CredentialRefFlagName, err) + if called { + t.Fatal("invalid --profile must stop before the leaf RunE") + } + if _, set := keychain.GetCredentialRefOverride(); set { + t.Fatal("invalid --profile must not record a credential-ref override") } } -// TestWireCredentialRefSelection_ShadowingSubcommand regresses the -// cobra-doesn't-chain-PersistentPreRunE bug for --ref, mirroring the -// --backend guard. func TestWireCredentialRefSelection_ShadowingSubcommand(t *testing.T) { resetState(t) t.Setenv(keychain.CredentialRefEnvVar(), "") shadow := &cobra.Command{ - Use: "shadow-ref", + Use: "shadow-profile", PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { return WireCredentialRefSelection(cmd) }, @@ -70,49 +69,57 @@ func TestWireCredentialRefSelection_ShadowingSubcommand(t *testing.T) { rootCmd.AddCommand(shadow) defer removeChild(t, shadow) - rootCmd.SetArgs([]string{"shadow-ref", "leaf", "--ref", "google-readonly/acct-b"}) + rootCmd.SetArgs([]string{"shadow-profile", "leaf", "--profile", "acct-b"}) if err := rootCmd.Execute(); err != nil { t.Fatalf("Execute through shadowing PreRunE: %v", err) } v, set := keychain.GetCredentialRefOverride() if !set || v != "google-readonly/acct-b" { - t.Errorf("override = (%q, %v); want (\"google-readonly/acct-b\", true) — shadower's PreRunE failed to invoke WireCredentialRefSelection", v, set) + t.Errorf("override = (%q, %v); want (google-readonly/acct-b, true)", v, set) } } -// TestCredentialRef_SetCredentialShadowsPersistent documents the intentional -// exception to the inherit-everywhere rule: `set-credential` keeps its own -// local --ref (the write target), so it must resolve to a DIFFERENT *pflag.Flag -// than the root's persistent selector — while a read command inherits the -// canonical persistent one. A regression that dropped set-credential's local -// flag (or that made a read command shadow --ref) would flip these. -func TestCredentialRef_SetCredentialShadowsPersistent(t *testing.T) { - canonical := rootCmd.PersistentFlags().Lookup(rootutil.CredentialRefFlagName) +func TestProfile_InheritsPersistentOnRealCommandTree(t *testing.T) { + canonical := rootCmd.PersistentFlags().Lookup(rootutil.ProfileFlagName) if canonical == nil { - t.Fatalf("root persistent flag --%s not registered", rootutil.CredentialRefFlagName) + t.Fatalf("root persistent flag --%s not registered", rootutil.ProfileFlagName) } - var sc *cobra.Command - for _, c := range rootCmd.Commands() { - if c.Name() == "set-credential" { - sc = c - break + var walk func(*cobra.Command) + walk = func(cmd *cobra.Command) { + children := cmd.Commands() + if len(children) == 0 { + if got := cmd.Flag(rootutil.ProfileFlagName); got != canonical { + t.Errorf("%q: --%s = %p, want canonical %p", cmd.CommandPath(), rootutil.ProfileFlagName, got, canonical) + } + return + } + for _, child := range children { + walk(child) } } - if sc == nil { - t.Fatal("set-credential command not registered on rootCmd") - } - if got := sc.Flag(rootutil.CredentialRefFlagName); got == nil { - t.Fatalf("set-credential has no --%s", rootutil.CredentialRefFlagName) - } else if got == canonical { - t.Errorf("set-credential --%s resolved to the persistent flag; expected its own local shadow", rootutil.CredentialRefFlagName) + walk(rootCmd) +} + +func TestNoRefFlagOnRealCommandTree(t *testing.T) { + resetState(t) + + var walk func(*cobra.Command) + walk = func(cmd *cobra.Command) { + if f := cmd.Flag("ref"); f != nil { + t.Errorf("%q exposes removed --ref flag (%s)", cmd.CommandPath(), f.Usage) + } + for _, child := range cmd.Commands() { + walk(child) + } } + walk(rootCmd) +} - // A read command (no local --ref) must inherit the canonical persistent flag. - me := newProbeCmd("probe-ref-inherit") - rootCmd.AddCommand(me) - defer removeChild(t, me) - if got := me.Flag(rootutil.CredentialRefFlagName); got != canonical { - t.Errorf("read command --%s = %p, want canonical %p (unexpected shadow)", rootutil.CredentialRefFlagName, got, canonical) +func TestRefFlagIsRejectedBySetCredential(t *testing.T) { + resetState(t) + rootCmd.SetArgs([]string{"set-credential", "--ref", "google-readonly/work"}) + if err := rootCmd.Execute(); err == nil || !strings.Contains(err.Error(), "unknown flag") { + t.Fatalf("set-credential must reject removed --ref, got %v", err) } } diff --git a/internal/app/gro/root.go b/internal/app/gro/root.go index 1927164..1eab782 100644 --- a/internal/app/gro/root.go +++ b/internal/app/gro/root.go @@ -80,7 +80,7 @@ func init() { // Set custom version template to include commit and build date rootCmd.SetVersionTemplate("gro " + version.Info() + "\n") - // Global flags (verbose, no-color, backend, ref) + // Global flags (verbose, no-color, backend, profile) rootutil.AddGlobalFlags(rootCmd, &verbose, &noColor) // Register commands diff --git a/internal/app/grw/profile_wire_test.go b/internal/app/grw/profile_wire_test.go new file mode 100644 index 0000000..b616a07 --- /dev/null +++ b/internal/app/grw/profile_wire_test.go @@ -0,0 +1,95 @@ +package grw + +import ( + "strings" + "testing" + + cccredstore "github.com/open-cli-collective/cli-common/credstore" + "github.com/spf13/cobra" + + "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/rootutil" +) + +func resetProfileTestState(t *testing.T) { + t.Helper() + config.Register(Identity()) + keychain.SetCredentialRefOverride("", false) + keychain.SetBackendFlagOverride("", false) + for _, name := range []string{rootutil.ProfileFlagName, cccredstore.BackendFlagName} { + if f := rootCmd.PersistentFlags().Lookup(name); f != nil { + _ = f.Value.Set(f.DefValue) + f.Changed = false + } + } + t.Cleanup(func() { + keychain.SetCredentialRefOverride("", false) + keychain.SetBackendFlagOverride("", false) + rootCmd.SetArgs(nil) + }) +} + +func TestProfileSelectionQualifiesGrwService(t *testing.T) { + resetProfileTestState(t) + + probe := &cobra.Command{Use: "probe-profile", RunE: func(*cobra.Command, []string) error { return nil }} + rootCmd.AddCommand(probe) + t.Cleanup(func() { rootCmd.RemoveCommand(probe) }) + rootCmd.SetArgs([]string{"probe-profile", "--profile", "work"}) + if err := rootCmd.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + if got, set := keychain.GetCredentialRefOverride(); !set || got != "google-readwrite/work" { + t.Errorf("override = (%q, %v), want (google-readwrite/work, true)", got, set) + } +} + +func TestInvalidProfileStopsBeforeLeaf(t *testing.T) { + resetProfileTestState(t) + called := false + probe := &cobra.Command{ + Use: "probe-profile-sentinel", + RunE: func(*cobra.Command, []string) error { + called = true + return nil + }, + } + rootCmd.AddCommand(probe) + t.Cleanup(func() { rootCmd.RemoveCommand(probe) }) + rootCmd.SetArgs([]string{"probe-profile-sentinel", "--profile", "bad.profile"}) + + err := rootCmd.Execute() + if err == nil || !strings.Contains(err.Error(), "--"+rootutil.ProfileFlagName) { + t.Fatalf("expected invalid --profile error, got %v", err) + } + if called { + t.Fatal("invalid --profile must stop before the leaf RunE") + } + if _, set := keychain.GetCredentialRefOverride(); set { + t.Fatal("invalid --profile must not record a credential-ref override") + } +} + +func TestNoRefFlagOnRealCommandTree(t *testing.T) { + resetProfileTestState(t) + + var walk func(*cobra.Command) + walk = func(cmd *cobra.Command) { + if f := cmd.Flag("ref"); f != nil { + t.Errorf("%q exposes removed --ref flag (%s)", cmd.CommandPath(), f.Usage) + } + for _, child := range cmd.Commands() { + walk(child) + } + } + walk(rootCmd) +} + +func TestRefFlagIsRejectedBySetCredential(t *testing.T) { + resetProfileTestState(t) + rootCmd.SetArgs([]string{"set-credential", "--ref", "google-readwrite/work"}) + if err := rootCmd.Execute(); err == nil || !strings.Contains(err.Error(), "unknown flag") { + t.Fatalf("set-credential must reject removed --ref, got %v", err) + } +} diff --git a/internal/app/grw/root.go b/internal/app/grw/root.go index eb8b3f6..6cdff5a 100644 --- a/internal/app/grw/root.go +++ b/internal/app/grw/root.go @@ -68,7 +68,7 @@ func ExecuteContext(ctx context.Context) { func init() { rootCmd.SetVersionTemplate("grw " + version.Info() + "\n") - // Global flags (verbose, no-color, backend, ref) + // Global flags (verbose, no-color, backend, profile) rootutil.AddGlobalFlags(rootCmd, &verbose, &noColor) // Register commands diff --git a/internal/cmd/init/init.go b/internal/cmd/init/init.go index 3864fdc..2e15bf7 100644 --- a/internal/cmd/init/init.go +++ b/internal/cmd/init/init.go @@ -37,7 +37,6 @@ type initOptions struct { noBrowser bool noVerify bool authCodeStdin bool - profile string } // NewCommand returns the init command. @@ -73,11 +72,6 @@ You can also copy your credentials.json to the clipboard and run init — it wil read, validate, and write it to the config directory for you.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - if opts.profile != "" { - if _, err := applyProfileFlag(opts.profile); err != nil { - return err - } - } return runWith(cmd.Context(), defaultDeps(), opts) }, } @@ -86,31 +80,9 @@ 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 { @@ -149,12 +121,12 @@ type initDeps struct { EnsureMigrated func() error // DescribeTarget names the credential ref this run will (re)authenticate, - // how it was selected (human label), and the cached account email it + // how it was selected, and the cached account email it // currently holds ("" when unknown). Backs the up-front announcement — // init touching "whatever the ref points at" without saying so is how an // intended add-an-account run silently overwrites the active profile's // token. Injected so tests can pin the announcement without a keyring. - DescribeTarget func() (ref, sourceLabel, cachedEmail string) + DescribeTarget func() (ref string, source config.RefSource, cachedEmail string) // RecordIdentity caches the verified account email for the active // profile (best-effort; feeds `profiles list`). Injected for tests. @@ -260,27 +232,27 @@ func defaultDeps() initDeps { } // describeTarget resolves the ref this run will authenticate (per-invocation -// overrides included, since OpenNoMigrate applies them), its source label, +// overrides included, since OpenNoMigrate applies them), its source, // and the cached account email. Best-effort: if the keyring can't open, fall // back to the loaded config's intent so the announcement still names a ref. -func describeTarget() (ref, sourceLabel, cachedEmail string) { +func describeTarget() (ref string, source config.RefSource, cachedEmail string) { st, err := keychain.OpenNoMigrate() if err != nil { cfg, cerr := config.LoadConfigForRuntime() if cerr != nil { return "", "", "" } - return cfg.CredentialRef, keychain.DescribeRefSource(cfg.CredentialRefSource()), "" + return cfg.CredentialRef, cfg.CredentialRefSource(), "" } defer func() { _ = st.Close() }() ref = st.Ref() - sourceLabel = keychain.DescribeRefSource(st.RefSource()) + source = st.RefSource() if _, profile, perr := credstore.ParseRef(ref); perr == nil { if e, ok := identitycache.Load()[profile]; ok { cachedEmail = e.Email } } - return ref, sourceLabel, cachedEmail + return ref, source, cachedEmail } // recordIdentity caches the verified email under the active profile so @@ -411,11 +383,17 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { // init (re)authenticating "whatever the ref points at" without saying // which profile/account is how an intended add-an-account run silently // overwrites the active profile's token. - var targetRef, target string + var targetRef, target, targetProfile string if d.DescribeTarget != nil { - ref, sourceLabel, cachedEmail := d.DescribeTarget() - if opts.profile != "" { - sourceLabel = "--profile flag" + ref, source, cachedEmail := d.DescribeTarget() + sourceLabel := "" + if source != "" { + sourceLabel = keychain.DescribeRefSource(source) + } + if source == config.RefSourceFlag { + if _, profile, err := credstore.ParseRef(ref); err == nil { + targetProfile = profile + } } targetRef = ref target = ref @@ -430,7 +408,7 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { d.View.Printf("Currently holds: %s\n", sanitize.Output(cachedEmail)) target = fmt.Sprintf("%s (%s)", ref, sanitize.Output(cachedEmail)) } - if opts.profile == "" { + if targetProfile == "" { d.View.Printf("To add a different account instead, use '%s init --profile '.\n", config.ProductName()) } d.View.Println("") @@ -456,7 +434,7 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { return err } if handled { - return finishRun(d, opts, targetRef) + return finishRun(d, targetProfile, targetRef) } // Step 4: OAuth flow. @@ -555,22 +533,22 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { d.View.Printf(" %s me\n", prod) } d.View.Printf(" %s mail search \"is:unread\"\n", prod) - return finishRun(d, opts, targetRef) + return finishRun(d, targetProfile, targetRef) } // finishRun appends the not-the-active-profile guidance after a successful // --profile run: the new account is authenticated but deliberately NOT made // active — adding an account must not hijack the default — so the user needs // to be told how to reach it. -func finishRun(d initDeps, opts *initOptions, targetRef string) error { - if opts.profile == "" || targetRef == "" { +func finishRun(d initDeps, profile, targetRef string) error { + if profile == "" || targetRef == "" { return nil } prod := config.ProductName() 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("Make it active: %s profiles use %s\n", prod, profile) + d.View.Printf("Use per invocation: %s --profile %s \n", prod, profile) return nil } diff --git a/internal/cmd/init/init_test.go b/internal/cmd/init/init_test.go index 903e606..1df4508 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" ) @@ -1041,41 +1040,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. @@ -1085,8 +1049,8 @@ func TestRunWithAnnouncesTarget(t *testing.T) { d := baseDeps(t, fs) out := &bytes.Buffer{} d.View = view.NewWithWriters(out, out) - d.DescribeTarget = func() (string, string, string) { - return "google-readonly/default", "config.yml credential_ref", "ada@example.com" + d.DescribeTarget = func() (string, config.RefSource, string) { + return "google-readonly/default", config.RefSourceConfig, "ada@example.com" } srcDir := t.TempDir() @@ -1119,8 +1083,8 @@ func TestReauthPromptNamesTarget(t *testing.T) { t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) - d.DescribeTarget = func() (string, string, string) { - return "google-readonly/default", "config.yml credential_ref", "ada@example.com" + d.DescribeTarget = func() (string, config.RefSource, string) { + return "google-readonly/default", config.RefSourceConfig, "ada@example.com" } d.HasStoredToken = func() bool { return true } calls := 0 @@ -1209,8 +1173,8 @@ func TestRunWithProfileFlagGuidance(t *testing.T) { d := baseDeps(t, fs) out := &bytes.Buffer{} d.View = view.NewWithWriters(out, out) - d.DescribeTarget = func() (string, string, string) { - return "google-readonly/work", "--ref flag", "" + d.DescribeTarget = func() (string, config.RefSource, string) { + return "google-readonly/work", config.RefSourceFlag, "" } srcDir := t.TempDir() @@ -1220,7 +1184,7 @@ func TestRunWithProfileFlagGuidance(t *testing.T) { } d.Prompter = &stubPrompter{redirectURL: "http://localhost/?code=ABC"} - if err := runWith(context.Background(), d, &initOptions{credentialsFile: src, noBrowser: true, profile: "work"}); err != nil { + if err := runWith(context.Background(), d, &initOptions{credentialsFile: src, noBrowser: true}); err != nil { t.Fatalf("runWith: %v", err) } got := out.String() @@ -1228,7 +1192,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..34d1fd5 100644 --- a/internal/cmd/profiles/profiles.go +++ b/internal/cmd/profiles/profiles.go @@ -38,7 +38,7 @@ 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 in config.yml (overridable per invocation with --profile or the _CREDENTIAL_REF environment variable).`, } cmd.AddCommand(newListCommand()) @@ -186,7 +186,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) @@ -226,7 +226,7 @@ account every subsequent command uses. Accepts a bare profile name ("work") or a full / ref for this CLI. Switching never touches tokens: the previous profile's token stays stored -and can be switched back to at any time. A per-invocation --ref flag or the +and can be switched back to at any time. A per-invocation --profile flag or the _CREDENTIAL_REF environment variable still takes precedence over the switched binding.`, Args: cobra.ExactArgs(1), diff --git a/internal/cmd/setcred/setcred.go b/internal/cmd/setcred/setcred.go index a6a5f31..51480fe 100644 --- a/internal/cmd/setcred/setcred.go +++ b/internal/cmd/setcred/setcred.go @@ -19,7 +19,6 @@ import ( ) type options struct { - ref string key string stdin bool fromEnv string @@ -48,7 +47,6 @@ flag or positional argument. Only the key 'oauth_token' is accepted. return run(opts) }, } - cmd.Flags().StringVar(&opts.ref, "ref", "", "Credential ref (default: config.yml credential_ref)") cmd.Flags().StringVar(&opts.key, "key", "", "Key to set: oauth_token") cmd.Flags().BoolVar(&opts.stdin, "stdin", false, "Read the token from stdin") cmd.Flags().StringVar(&opts.fromEnv, "from-env", "", "Read the token from this env var") @@ -83,29 +81,10 @@ 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). - // 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). - migrated := false - if opts.ref == "" { - if merr := keychain.EnsureMigrated(); merr != nil { - return merr - } - migrated = true - } - - st, err := keychain.OpenRef(opts.ref) // ingress: runMigration=false + // keychain.Open applies the inherited --profile selector and keeps the + // one-time migration scoped to the configured/default profile. + st, err := keychain.Open() if err != nil { - if migrated { - // The legacy original may already have been consumed by the - // migration above; make the failure actionable. - return fmt.Errorf("legacy migration succeeded but the keyring write could not be opened (run 'the CLI init' to re-authenticate): %w", err) - } return err } defer func() { _ = st.Close() }() diff --git a/internal/cmd/setcred/setcred_test.go b/internal/cmd/setcred/setcred_test.go index 16a9801..279ffc3 100644 --- a/internal/cmd/setcred/setcred_test.go +++ b/internal/cmd/setcred/setcred_test.go @@ -1,15 +1,59 @@ package setcred import ( + "errors" + "os" + "path/filepath" "strings" "testing" + "github.com/open-cli-collective/cli-common/credstore" + "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" ) const tokenJSON = `{"access_token":"SECRET-ACCESS","refresh_token":"SECRET-REFRESH","token_type":"Bearer"}` +func writeLegacyToken(t *testing.T, value string) string { + t.Helper() + path, err := config.GetTokenPath() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(value), 0o600); err != nil { + t.Fatal(err) + } + return path +} + +func seedTokenAtRef(t *testing.T, ref, access string) { + t.Helper() + st, err := keychain.OpenRef(ref) + if err != nil { + t.Fatal(err) + } + defer func() { _ = st.Close() }() + if err := st.SetToken(&oauth2.Token{AccessToken: access, RefreshToken: "seed-refresh"}); err != nil { + t.Fatal(err) + } +} + +func tokenAtRef(t *testing.T, ref string) (*oauth2.Token, error) { + t.Helper() + st, err := keychain.OpenRef(ref) + if err != nil { + return nil, err + } + defer func() { _ = st.Close() }() + return st.Token() +} + func TestSetCredentialStdin(t *testing.T) { credtest.Setup(t) err := run(&options{key: keychain.KeyOAuthToken, stdin: true, in: strings.NewReader(tokenJSON)}) @@ -27,6 +71,28 @@ func TestSetCredentialStdin(t *testing.T) { } } +func TestSetCredentialUsesInheritedProfile(t *testing.T) { + credtest.Setup(t) + seedTokenAtRef(t, config.DefaultCredentialRef, "DEFAULT-ACCESS") + + keychain.SetCredentialRefOverride("google-readonly/work", 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 --profile work: %v", err) + } + + keychain.SetCredentialRefOverride("", false) + defaultToken, err := tokenAtRef(t, config.DefaultCredentialRef) + if err != nil || defaultToken.AccessToken != "DEFAULT-ACCESS" { + t.Fatalf("configured profile token changed: %+v err=%v", defaultToken, err) + } + + workToken, err := tokenAtRef(t, "google-readonly/work") + if err != nil || workToken.AccessToken != "SECRET-ACCESS" || workToken.RefreshToken != "SECRET-REFRESH" { + t.Fatalf("selected profile token missing: %+v err=%v", workToken, err) + } +} + func TestSetCredentialKeyAllowlist(t *testing.T) { credtest.Setup(t) err := run(&options{key: "not_allowed", stdin: true, in: strings.NewReader(tokenJSON)}) @@ -67,3 +133,42 @@ func TestSetCredentialRejectsNonToken(t *testing.T) { t.Fatalf("want token-shape rejection, got %v", err) } } + +func TestSetCredentialMigrationConflictBlocksWrite(t *testing.T) { + credtest.Setup(t) + seedTokenAtRef(t, config.DefaultCredentialRef, "KEYRING") + legacyPath := writeLegacyToken(t, `{"access_token":"LEGACY","refresh_token":"LEGACY-REFRESH"}`) + + err := run(&options{key: keychain.KeyOAuthToken, stdin: true, in: strings.NewReader(tokenJSON)}) + if !errors.Is(err, credstore.ErrMigrationConflict) { + t.Fatalf("want migration conflict, got %v", err) + } + if _, statErr := os.Stat(legacyPath); statErr != nil { + t.Fatalf("legacy token must remain after conflict: %v", statErr) + } + tok, readErr := tokenAtRef(t, config.DefaultCredentialRef) + if readErr != nil || tok.AccessToken != "KEYRING" { + t.Fatalf("target changed after conflict: %+v, err=%v", tok, readErr) + } +} + +func TestSetCredentialExplicitProfileDoesNotMigrateDefaultLegacy(t *testing.T) { + credtest.Setup(t) + legacyPath := writeLegacyToken(t, `{"access_token":"LEGACY","refresh_token":"LEGACY-REFRESH"}`) + keychain.SetCredentialRefOverride("google-readonly/work", 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 --profile work: %v", err) + } + if _, err := os.Stat(legacyPath); err != nil { + t.Fatalf("explicit profile must not consume default legacy token: %v", err) + } + selected, err := tokenAtRef(t, "google-readonly/work") + if err != nil || selected.AccessToken != "SECRET-ACCESS" { + t.Fatalf("selected profile token = %+v, err=%v", selected, err) + } + if _, err := tokenAtRef(t, config.DefaultCredentialRef); !errors.Is(err, keychain.ErrTokenNotFound) { + t.Fatalf("default profile should remain unmigrated, got err=%v", err) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index bd2d62c..4984795 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -91,8 +91,8 @@ type Config struct { type RefSource string // RefSource values, in precedence order (flag > env > config > default). -// RefSourceExplicit marks a caller-supplied ref (set-credential --ref, the -// refresh persister) that bypassed the precedence chain. +// RefSourceExplicit marks a caller-supplied ref (the refresh persister) that +// bypassed the precedence chain. const ( RefSourceFlag RefSource = "flag" RefSourceEnv RefSource = "env" diff --git a/internal/keychain/credref_test.go b/internal/keychain/credref_test.go index 320b387..369b959 100644 --- a/internal/keychain/credref_test.go +++ b/internal/keychain/credref_test.go @@ -6,7 +6,7 @@ import ( "github.com/open-cli-collective/google-cli/internal/config" ) -// resetCredRefOverride keeps the package-level --ref override clean across +// resetCredRefOverride keeps the package-level --profile override clean across // tests so a leaked value can't tilt the next. func resetCredRefOverride(t *testing.T) { t.Helper() @@ -22,7 +22,7 @@ func TestCredentialRefEnvVar(t *testing.T) { } } -// TestEffectiveRef_Precedence proves --ref flag > env > config, and that an +// TestEffectiveRef_Precedence proves --profile flag > env > config, and that an // override is reported (so the caller suppresses the one-time migration). func TestEffectiveRef_Precedence(t *testing.T) { const cfgRef = "google-readonly/cfg" @@ -61,7 +61,7 @@ func TestEffectiveRef_Precedence(t *testing.T) { SetCredentialRefOverride("", true) // Changed=true but no value ref, _, ov := effectiveRef(cfgRef) if ref != cfgRef || ov { - t.Errorf("got (%q,%v), want (%q,false) — empty --ref must fall through", ref, ov, cfgRef) + t.Errorf("got (%q,%v), want (%q,false) — empty --profile must fall through", ref, ov, cfgRef) } }) } @@ -69,7 +69,7 @@ func TestEffectiveRef_Precedence(t *testing.T) { // TestApplyCredentialRefOverride proves the safety-critical part open() relies // on: a present override swaps cfg.CredentialRef AND forces runMigration=false // (so the one-time legacy migration never runs against an arbitrary -// --ref/env-selected profile), while no override leaves both untouched. This is +// --profile/env-selected profile), while no override leaves both untouched. This is // the open()-side coverage the pure effectiveRef/wiring tests don't provide. func TestApplyCredentialRefOverride(t *testing.T) { const cfgRef = "google-readonly/cfg" @@ -129,7 +129,7 @@ func TestDescribeRefSource(t *testing.T) { src config.RefSource want string }{ - {config.RefSourceFlag, "--ref flag"}, + {config.RefSourceFlag, "--profile flag"}, {config.RefSourceEnv, "GOOGLE_READONLY_CREDENTIAL_REF environment variable"}, {config.RefSourceConfig, "config.yml credential_ref"}, {config.RefSourceDefault, "built-in default; config.yml sets no credential_ref"}, diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 3eacc80..eec59e5 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -78,9 +78,9 @@ func open(overwrite, runMigration bool) (*Store, error) { return openWith(cfg, overwrite, runMigration) } -// applyCredentialRefOverride applies the per-invocation credential-ref override +// applyCredentialRefOverride applies the per-invocation profile override // to cfg in place and returns the effective runMigration decision. A -// --ref flag or _CREDENTIAL_REF env (precedence: flag > env > config) +// --profile flag or _CREDENTIAL_REF env (precedence: flag > env > config) // selects the ref for this process, so concurrent processes can each target a // different account without racing on the shared config.yml. A present override // also FORCES runMigration=false: the one-time §1.8 migration only ever targets @@ -103,7 +103,7 @@ func applyCredentialRefOverride(cfg *config.Config, runMigration bool) bool { // (e.g. "GOOGLE_READONLY_CREDENTIAL_REF"). It is derived from gro's service so // it always tracks the same _ prefix credstore uses for the backend // env var, and is never hard-coded (§1.3). Exported so the cobra layer can name -// it in the --ref flag's help text. +// it in the --profile flag's help text. func CredentialRefEnvVar() string { service, _, err := credstore.ParseRef(config.DefaultCredentialRef) if err != nil { @@ -114,7 +114,7 @@ func CredentialRefEnvVar() string { } // effectiveRef applies the per-invocation credential-ref precedence -// (--ref flag > _CREDENTIAL_REF env > config credential_ref) and +// (--profile flag > _CREDENTIAL_REF env > config credential_ref) and // reports whether an explicit override was supplied, plus which source won // (for error attribution). Mirrors the --backend precedence chain. When // overridden is true, the caller must skip the one-time §1.8 migration (see @@ -130,10 +130,10 @@ func effectiveRef(configRef string) (ref string, source config.RefSource, overri } // OpenRef opens a store against an explicit ref instead of config.yml's -// credential_ref — used by `gro set-credential --ref` and the refresh +// credential_ref — used by the refresh // persister. An empty ref falls back to the configured/default ref. // Migration does NOT run here: the one-time §1.8 migration only ever targets -// the canonical configured ref (running it against an arbitrary --ref would +// the canonical configured ref (running it against an arbitrary --profile would // discover the default ref's legacy data and could write it under the wrong // service/profile). func OpenRef(ref string) (*Store, error) { @@ -216,7 +216,7 @@ func (s *Store) RefSource() config.RefSource { return s.refSource } func DescribeRefSource(s config.RefSource) string { switch s { case config.RefSourceFlag: - return "--ref flag" + return "--profile flag" case config.RefSourceEnv: return CredentialRefEnvVar() + " environment variable" case config.RefSourceConfig: diff --git a/internal/keychain/wire.go b/internal/keychain/wire.go index 013901a..d1e3cd6 100644 --- a/internal/keychain/wire.go +++ b/internal/keychain/wire.go @@ -35,13 +35,13 @@ var ( credRefFlagWasSet bool ) -// SetCredentialRefOverride records the user-supplied --ref flag for the next +// SetCredentialRefOverride records the service-qualified --profile selection for the next // keychain.Open* call. Called by root.WireCredentialRefSelection at // PersistentPreRunE time. Mirrors SetBackendFlagOverride: a persistent flag // can't be threaded through the parameterless keychain.Open() the read // commands call, so it is recorded here and read back at the single // resolution site (open). flagSet matches cobra's pflag.Flag.Changed — true -// when the user passed --ref on the command line, regardless of value. +// when the user passed --profile on the command line, regardless of value. func SetCredentialRefOverride(value string, flagSet bool) { credRefMu.Lock() defer credRefMu.Unlock() @@ -49,7 +49,7 @@ func SetCredentialRefOverride(value string, flagSet bool) { credRefFlagWasSet = flagSet } -// GetCredentialRefOverride returns the current --ref override and whether it +// GetCredentialRefOverride returns the current profile override and whether it // was set. The flag-set vs unset distinction lets an explicit empty value be // told apart from "no flag". func GetCredentialRefOverride() (value string, flagSet bool) { diff --git a/internal/rootutil/rootutil.go b/internal/rootutil/rootutil.go index 2bd629f..1105ff2 100644 --- a/internal/rootutil/rootutil.go +++ b/internal/rootutil/rootutil.go @@ -1,7 +1,7 @@ // Package rootutil holds the root-command scaffolding shared by every CLI built // on this library: the standard global flags (--verbose, --no-color, -// --backend, --ref), the PersistentPreRunE wiring that records the -// backend/credential-ref selection for the next keychain.Open call, and the +// --backend, --profile), the PersistentPreRunE wiring that records the +// backend/profile selection for the next keychain.Open call, and the // migration-notice flush that must wrap execution. Each CLI's root package // supplies its own Use/Short/Long and command set and calls these helpers, so // the plumbing lives in exactly one place. @@ -18,33 +18,32 @@ 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" ) -// CredentialRefFlagName is the global per-invocation credential-ref selector. -// It shares its name with set-credential's own write-target --ref (a local flag -// that shadows this persistent one for that command only). -const CredentialRefFlagName = "ref" +// ProfileFlagName is the global per-invocation credential-profile selector. +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 +// verbose and noColor to the given pointers. The --profile help shows the env var // by its _ pattern rather than the resolved name because flags are // registered at package-init time, before config.Register runs. func AddGlobalFlags(cmd *cobra.Command, verbose, noColor *bool) { cmd.PersistentFlags().BoolVarP(verbose, "verbose", "v", false, "Enable verbose output for debugging") cmd.PersistentFlags().BoolVar(noColor, "no-color", false, "Disable colored output") cmd.PersistentFlags().String(cccredstore.BackendFlagName, "", cccredstore.BackendFlagUsage()) - cmd.PersistentFlags().String(CredentialRefFlagName, "", fmt.Sprintf( - "Credential ref / for this invocation, so concurrent commands "+ + cmd.PersistentFlags().String(ProfileFlagName, "", fmt.Sprintf( + "Select profile for this invocation, so concurrent commands "+ "can target different accounts without racing on config.yml "+ "(precedence: --%s flag > _CREDENTIAL_REF env > config credential_ref)", - CredentialRefFlagName)) + ProfileFlagName)) } // ApplyGlobalFlags runs the shared PersistentPreRunE logic: verbosity, color, -// and backend/ref wiring. Each root calls it from its own PersistentPreRunE. +// and backend/profile wiring. Each root calls it from its own PersistentPreRunE. func ApplyGlobalFlags(cmd *cobra.Command, verbose, noColor bool) error { log.Verbose = verbose if noColor { @@ -77,21 +76,26 @@ 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 qualifies the user-supplied --profile name with +// the registered service and records it for the next keychain.Open* call. The +// credstore formatter validates the bare name before any keyring work. The +// resolved precedence (--profile flag > _CREDENTIAL_REF env > config +// credential_ref) is applied at keychain.open. func WireCredentialRefSelection(cmd *cobra.Command) error { - f := cmd.Flag(CredentialRefFlagName) + f := cmd.Flag(ProfileFlagName) if f == nil { return nil } value := f.Value.String() changed := f.Changed - if changed && value != "" { - if _, _, err := cccredstore.ParseRef(value); err != nil { - return fmt.Errorf("--%s: %w", CredentialRefFlagName, err) + if changed { + service, _, err := cccredstore.ParseRef(config.DefaultCredentialRef) + if err != nil { + return fmt.Errorf("invalid default credential ref: %w", err) + } + value, err = cccredstore.FormatRef(service, value) + if err != nil { + return fmt.Errorf("--%s: %w", ProfileFlagName, err) } } keychain.SetCredentialRefOverride(value, changed) diff --git a/version.txt b/version.txt index 5625e59..cd5ac03 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -1.2 +2.0