diff --git a/README.md b/README.md index dacfbfb..9d589b5 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ datumctl get dnszones --session alice@example.com@api.staging.env.datum.net DATUM_SESSION=alice@example.com datumctl get projects ``` -datumctl also sets `DATUM_SESSION` for plugins it runs, so a plugin's own `datumctl` calls act as the same session. A stale `DATUM_SESSION` — left over from a previous logout, or inherited this way from a plugin — only affects commands that need a session; `datumctl version`, `datumctl plugin list`, and similar commands are unaffected. +This works on plugin commands too (`datumctl dns zones list --session alice@example.com@api.datum.net`): datumctl takes the flag itself and sets `DATUM_SESSION` for the plugin it runs, so the plugin's own `datumctl` calls act as the same session. A stale `DATUM_SESSION` — left over from a previous logout, or inherited this way from a plugin — only affects commands that need a session; `datumctl version`, `datumctl plugin list`, and similar commands are unaffected. For machine-to-machine auth, see `datumctl login --credentials` for the machine-account flow. diff --git a/internal/plugindispatch/forward.go b/internal/plugindispatch/forward.go index 2a53da1..cc32aa4 100644 --- a/internal/plugindispatch/forward.go +++ b/internal/plugindispatch/forward.go @@ -15,6 +15,7 @@ import ( "go.datum.net/datumctl/internal/client" "go.datum.net/datumctl/internal/datumconfig" + customerrors "go.datum.net/datumctl/internal/errors" "go.datum.net/datumctl/internal/pluginstore" ) @@ -35,12 +36,12 @@ import ( // can correctly distinguish plugin names from registered subcommands. // On success this function does not return (process is replaced). // -// A leading global --session flag ("datumctl --session X ...") or the -// DATUM_SESSION environment variable runs the plugin as that session. When the -// session cannot be resolved, ForwardPlugin returns the UserError so the caller -// can report it; the plugin is not run. +// The global --session flag, wherever the user put it on the command line, or +// the DATUM_SESSION environment variable runs the plugin as that session. When +// the session cannot be resolved, ForwardPlugin returns the UserError so the +// caller can report it; the plugin is not run. func ForwardPlugin(pluginsDir string, root *cobra.Command, factory *client.DatumCloudFactory) error { - sessionValue, args, hasSessionFlag := splitLeadingSessionFlag(os.Args[1:]) + sessionValue, args, hasSessionFlag := splitSessionFlag(os.Args[1:]) if len(args) == 0 || strings.HasPrefix(args[0], "-") { return nil } @@ -96,22 +97,35 @@ func ForwardPlugin(pluginsDir string, root *cobra.Command, factory *client.Datum return Exec(binaryPath, args[1:], factory) } -// splitLeadingSessionFlag removes global --session flags that appear before -// the plugin name ("--session X" or "--session=X") and returns the last value -// given, the remaining arguments, and whether the flag was present. Arguments -// after the plugin name belong to the plugin and are left alone. -func splitLeadingSessionFlag(args []string) (value string, rest []string, found bool) { - for len(args) > 0 { +// splitSessionFlag removes datumctl's global --session flag ("--session X" or +// "--session=X") from a plugin command line and returns the last value given, +// the remaining arguments, and whether the flag was present. +// +// The flag is taken from anywhere in the command line, not just before the +// plugin name: --session belongs to datumctl, and plugins do not define it, so +// leaving one in place makes the plugin refuse the whole command with its own +// "unknown flag: --session". +// +// Everything after a bare "--" is the plugin's to interpret and is left alone. +func splitSessionFlag(args []string) (value string, rest []string, found bool) { + rest = make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { switch { - case args[0] == "--session" && len(args) > 1: - value, found, args = args[1], true, args[2:] - case strings.HasPrefix(args[0], "--session="): - value, found, args = strings.TrimPrefix(args[0], "--session="), true, args[1:] + case args[i] == "--": + return value, append(rest, args[i:]...), found + case args[i] == "--session": + found = true + if i+1 < len(args) { + value = args[i+1] + i++ + } + case strings.HasPrefix(args[i], "--session="): + value, found = strings.TrimPrefix(args[i], "--session="), true default: - return value, args, found + rest = append(rest, args[i]) } } - return value, args, found + return value, rest, found } // applyPluginSessionOverride installs the session a plugin runs as: the @@ -126,6 +140,15 @@ func splitLeadingSessionFlag(args []string) (value string, rest []string, found // the unrelated real active session. func applyPluginSessionOverride(flagValue string, hasFlag bool) error { if hasFlag { + // --session with nothing after it would otherwise clear the override and + // quietly run as the active session, which is the mistake the flag exists + // to prevent. + if strings.TrimSpace(flagValue) == "" { + return customerrors.NewUserErrorWithHint( + "--session needs a session name.", + "Run 'datumctl auth list' to see the available session names.", + ) + } _, err := datumconfig.ApplySessionOverride(flagValue, datumconfig.SessionOverrideFromFlag) return err } @@ -220,7 +243,13 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err return nil } - name := os.Args[2] + // Take --session out of the completion request the same way the run path + // does, so it never reaches the plugin as an argument it does not know. + sessionValue, completionArgs, hasSessionFlag := splitSessionFlag(os.Args[2:]) + if len(completionArgs) == 0 { + return nil + } + name := completionArgs[0] // Find the plugin binary. binaryPath, managed, err := FindPlugin(name, pluginsDir) @@ -253,7 +282,7 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err // Strip the plugin name from the forwarded args so the plugin sees // ["__complete", ] rather than ["__complete", "compute", ]. // The plugin's own cobra tree has no knowledge of its name as a subcommand. - pluginArgs := append([]string{"__complete"}, os.Args[3:]...) + pluginArgs := append([]string{"__complete"}, completionArgs[1:]...) cmd := exec.Command(binaryPath, pluginArgs...) cmd.Stdin = os.Stdin @@ -264,9 +293,10 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err // completion. Non-fatal: if env construction fails we proceed without injection // and the plugin will return empty candidates instead of erroring. if factory != nil { - // Honor DATUM_SESSION so completion lists what the plugin will act on. - // An unresolvable value is ignored here; running the plugin reports it. - _ = applyPluginSessionOverride("", false) + // Honor --session/DATUM_SESSION so completion lists what the plugin will + // act on. An unresolvable value is ignored here; running the plugin + // reports it. + _ = applyPluginSessionOverride(sessionValue, hasSessionFlag) if env, buildErr := BuildEnv(factory); buildErr == nil { cmd.Env = overlayEnv(os.Environ(), env) } @@ -294,9 +324,9 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err // Cobra intercepts --help before RunE fires, so this must be called before // cobra.Execute(), mirroring the ForwardCompletion pattern. func ForwardHelp(pluginsDir string) error { - // Strip "datumctl" and any leading --session, so help for - // "datumctl --session X --help" reaches the plugin too. - _, args, _ := splitLeadingSessionFlag(os.Args[1:]) + // Strip "datumctl" and any --session, so help for + // "datumctl --session X --help" reaches the plugin too. + _, args, _ := splitSessionFlag(os.Args[1:]) if len(args) == 0 { return nil } diff --git a/internal/plugindispatch/session_override_test.go b/internal/plugindispatch/session_override_test.go index 04df42a..795903f 100644 --- a/internal/plugindispatch/session_override_test.go +++ b/internal/plugindispatch/session_override_test.go @@ -2,6 +2,7 @@ package plugindispatch import ( "context" + "os" "slices" "strings" "testing" @@ -10,6 +11,7 @@ import ( "go.datum.net/datumctl/internal/datumconfig" customerrors "go.datum.net/datumctl/internal/errors" "go.datum.net/datumctl/internal/keyring" + "go.datum.net/datumctl/internal/pluginstore" ) const ( @@ -169,7 +171,7 @@ func TestApplyPluginSessionOverride_StaleEnvDoesNotError(t *testing.T) { } } -func TestSplitLeadingSessionFlag(t *testing.T) { +func TestSplitSessionFlag(t *testing.T) { tests := []struct { in []string wantValue string @@ -179,15 +181,88 @@ func TestSplitLeadingSessionFlag(t *testing.T) { {[]string{"ipam", "list"}, "", []string{"ipam", "list"}, false}, {[]string{"--session", "a@b@c", "ipam", "list", "-o", "wide"}, "a@b@c", []string{"ipam", "list", "-o", "wide"}, true}, {[]string{"--session=a@b", "ipam"}, "a@b", []string{"ipam"}, true}, - // A --session after the plugin name belongs to the plugin. - {[]string{"ipam", "--session", "x"}, "", []string{"ipam", "--session", "x"}, false}, + // Issue #304: --session after the plugin name is still datumctl's, and + // must not reach the plugin as an argument it does not recognize. + {[]string{"ipam", "--session", "x"}, "x", []string{"ipam"}, true}, + {[]string{"assistant", "card", "--session", "a@b@c"}, "a@b@c", []string{"assistant", "card"}, true}, + {[]string{"assistant", "card", "--session=a@b@c"}, "a@b@c", []string{"assistant", "card"}, true}, + {[]string{"ipam", "list", "--session", "a@b", "-o", "wide"}, "a@b", []string{"ipam", "list", "-o", "wide"}, true}, + // Missing value: reported as present with no value so the caller can say + // so, rather than silently running as the active session. + {[]string{"ipam", "list", "--session"}, "", []string{"ipam", "list"}, true}, + // Everything after a bare "--" is the plugin's to interpret. + {[]string{"ipam", "--", "--session", "x"}, "", []string{"ipam", "--", "--session", "x"}, false}, {[]string{"--project", "p", "ipam"}, "", []string{"--project", "p", "ipam"}, false}, } for _, tt := range tests { - v, rest, found := splitLeadingSessionFlag(tt.in) + v, rest, found := splitSessionFlag(tt.in) if v != tt.wantValue || found != tt.wantFound || !slices.Equal(rest, tt.wantRest) { - t.Errorf("splitLeadingSessionFlag(%q) = %q, %q, %v; want %q, %q, %v", + t.Errorf("splitSessionFlag(%q) = %q, %q, %v; want %q, %q, %v", tt.in, v, rest, found, tt.wantValue, tt.wantRest, tt.wantFound) } } } + +// Issue #304: "datumctl --session X" must run the plugin as +// that session. The flag is consumed by datumctl and turned into DATUM_SESSION +// (plus the matching API host) instead of being handed to the plugin, which +// would reject the whole command with "unknown flag: --session". +func TestForwardPlugin_sessionFlagAfterPluginName(t *testing.T) { + // Not parallel — mutates os.Args, execPlatform, and environment. + factory := setupPluginOverrideEnv(t) + managedDir := t.TempDir() + // Isolate PATH so only the managed binary resolves the plugin name. + t.Setenv("PATH", t.TempDir()) + + binaryPath := writeFakeBinary(t, managedDir, "assistant") + writeManifest(t, managedDir, &pluginstore.Manifest{ + Plugins: map[string]*pluginstore.InstalledPlugin{ + "assistant": {SHA256: sha256HexFile(t, binaryPath)}, + }, + }) + + var gotArgs, gotEnv []string + execCalled := false + origExec := execPlatform + execPlatform = func(_ string, args []string, env []string) error { + execCalled, gotArgs, gotEnv = true, args, env + return nil + } + t.Cleanup(func() { execPlatform = origExec }) + + origArgs := os.Args + os.Args = []string{"datumctl", "assistant", "card", "--session", ovStaging} + t.Cleanup(func() { os.Args = origArgs }) + + if err := ForwardPlugin(managedDir, buildMinimalCobraTree(), factory); err != nil { + t.Fatalf("ForwardPlugin: %v", err) + } + if !execCalled { + t.Fatal("plugin was not exec'd") + } + if want := []string{"card"}; !slices.Equal(gotArgs, want) { + t.Errorf("forwarded args = %v, want %v (--session must not reach the plugin)", gotArgs, want) + } + if got := envValue(gotEnv, "DATUM_SESSION"); got != ovStaging { + t.Errorf("DATUM_SESSION = %q, want %q", got, ovStaging) + } + if got := envValue(gotEnv, "DATUM_API_HOST"); got != "api.staging.env.datum.net" { + t.Errorf("DATUM_API_HOST = %q, want the override session's host", got) + } +} + +// A --session with no value must fail with datumctl's own message rather than +// falling through and running the plugin as the active session. +func TestApplyPluginSessionOverride_missingValue(t *testing.T) { + setupPluginOverrideEnv(t) + err := applyPluginSessionOverride("", true) + if err == nil { + t.Fatal("expected an error for --session with no value") + } + if _, ok := customerrors.IsUserError(err); !ok { + t.Errorf("error is not a UserError: %v", err) + } + if datumconfig.HasSessionOverride() { + t.Error("a failed override must not leave one installed") + } +}