From c2e194bc07d4f9d149041dd3e8c68ae259b58595 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Tue, 29 Sep 2026 17:26:04 -0500 Subject: [PATCH] fix: Accept --session on plugin commands "datumctl --session X" failed with the plugin's own "unknown flag: --session", because only a --session placed before the plugin name was consumed by datumctl; anywhere else it was forwarded as one of the plugin's arguments. That pointed users at the wrong component and pushed them toward switching their active session instead, which is exactly what the flag exists to avoid. datumctl now takes --session out of a plugin command line wherever it appears and turns it into the DATUM_SESSION (and API host) the plugin already understands. Arguments after a bare "--" still belong to the plugin. The flag continues to beat DATUM_SESSION, and --session with no value now fails with datumctl's own message rather than quietly running as the active session. Fixes #304 Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- internal/plugindispatch/forward.go | 80 +++++++++++------ .../plugindispatch/session_override_test.go | 85 +++++++++++++++++-- 3 files changed, 136 insertions(+), 31 deletions(-) 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") + } +}