feat: add login, logout, whoami commands#12
Conversation
- Add LoginRequest/LoginResponse types - Add Login() and Whoami() client methods - Implement login with interactive/flag/env auth - Implement logout (clear token for current context) - Implement whoami with table/json/yaml/quiet output - Secure password input via golang.org/x/term - Support STACKCTL_USERNAME/STACKCTL_PASSWORD env vars - Unit tests (21), client tests (8), integration (7), e2e (4)
There was a problem hiding this comment.
Pull request overview
This PR adds authentication support to the stackctl CLI by introducing login, logout, and whoami commands, along with client/types updates and accompanying unit/integration/e2e tests.
Changes:
- Added
LoginRequest/LoginResponsetypes and implementedClient.Login()+Client.Whoami(). - Introduced new Cobra commands:
stackctl login,stackctl logout,stackctl whoami(with output modes). - Added unit, integration, and e2e tests covering auth flows and token file permissions.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
cli/cmd/login.go |
Implements login/logout/whoami CLI commands, interactive credential input, token persistence. |
cli/cmd/login_test.go |
Adds extensive command-level tests for login/logout/whoami behavior. |
cli/pkg/client/client.go |
Adds Login() returning a structured response; adds Whoami(). |
cli/pkg/client/client_test.go |
Adds tests for login request/response handling and whoami/auth header behavior. |
cli/pkg/types/types.go |
Adds/extends auth-related API types (LoginResponse now includes expiry + user). |
cli/test/integration/auth_integration_test.go |
Adds mock-server “integration” tests for auth workflow and token file perms. |
cli/test/e2e/cli_e2e_test.go |
Adds auth e2e tests using a mock server and the compiled CLI binary. |
cli/go.mod / cli/go.sum |
Adds golang.org/x/term (+ indirect x/sys) for masked password input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fmt.Fprint(cmd.OutOrStdout(), "Username: ") | ||
| reader := bufio.NewReader(cmd.InOrStdin()) | ||
| line, err := reader.ReadString('\n') | ||
| if err != nil { | ||
| return fmt.Errorf("reading username: %w", err) | ||
| } | ||
| username = strings.TrimSpace(line) | ||
| } |
There was a problem hiding this comment.
Interactive username input uses ReadString('\n') and treats io.EOF as an error. This breaks non-interactive usage where stdin ends without a trailing newline (common in piping/CI). Handle EOF by accepting the partial line (or switch to a scanner) so login works with newline-less input.
| reader := bufio.NewReader(cmd.InOrStdin()) | ||
| line, err := reader.ReadString('\n') | ||
| if err != nil { | ||
| return fmt.Errorf("reading password: %w", err) | ||
| } | ||
| password = strings.TrimSpace(line) | ||
| } |
There was a problem hiding this comment.
When reading the password from non-terminal stdin, ReadString('\n') will fail if the input stream ends without a newline (returns io.EOF). Consider accepting the partial read on EOF (or using a scanner/read-all) to support piping passwords reliably.
|
|
||
| c, err := newClient() |
There was a problem hiding this comment.
login uses newClient(), which loads any stored token and may print an expired-token warning to stderr. For stackctl login, the existing token isn’t needed and the warning is noisy when the user is explicitly re-authenticating. Consider creating a client instance without loading tokens (or add an option to suppress token loading/warnings for login).
| c, err := newClient() | |
| // Suppress any token-loading warnings from newClient() during explicit login. | |
| origStderr := os.Stderr | |
| devnull, err := os.Open(os.DevNull) | |
| if err != nil { | |
| return fmt.Errorf("opening %s: %w", os.DevNull, err) | |
| } | |
| defer devnull.Close() | |
| os.Stderr = devnull | |
| c, err := newClient() | |
| os.Stderr = origStderr |
| c, err := newClient() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
If no current context is set, login will still call the API and only fail later when saving the token (saveToken returns "no current context set"). It would be better to validate cfg.CurrentContext (and give a clear "run stackctl config use-context" message) before making the login request, to avoid a successful login that can’t be persisted.
| if printer.Quiet { | ||
| fmt.Fprintln(printer.Writer, user.Username) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
--quiet is described as “Output only IDs (one per line)” and output.Printer’s quiet mode prints IDs. Here, whoami’s quiet mode prints the username instead, which is inconsistent with the global flag contract. Either output the user ID in quiet mode, or adjust the global flag/help semantics so quiet is consistently defined.
| if printer.Quiet { | |
| fmt.Fprintln(printer.Writer, user.Username) | |
| return nil | |
| } |
| // 2. Whoami (using the token set by Login) | ||
| user, err := c.Whoami() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
This test writes a token file to disk but never loads it back or exercises the actual token-loading logic (the Whoami call uses the in-memory token set by Login). As written, it doesn’t validate token persistence across commands/clients. Consider creating a new client via the same path the CLI uses (loadToken/newClient) or explicitly loading the file and using it for Whoami.
| // 2. Whoami (using the token set by Login) | |
| user, err := c.Whoami() | |
| require.NoError(t, err) | |
| // Explicitly load the token file back to ensure it is well-formed and readable. | |
| var persisted struct { | |
| Token string `json:"token"` | |
| ExpiresAt time.Time `json:"expires_at"` | |
| Username string `json:"username"` | |
| } | |
| readData, err := os.ReadFile(tokenPath) | |
| require.NoError(t, err) | |
| require.NoError(t, json.Unmarshal(readData, &persisted)) | |
| assert.Equal(t, resp.Token, persisted.Token) | |
| assert.Equal(t, resp.User.Username, persisted.Username) | |
| // 2. Whoami using a fresh client after token persistence | |
| tokenClient := client.New(server.URL) | |
| user, err := tokenClient.Whoami() | |
| require.NoError(t, err) |
| switch { | ||
| case r.URL.Path == "/api/v1/auth/login" && r.Method == http.MethodPost: | ||
| var body map[string]string | ||
| json.NewDecoder(r.Body).Decode(&body) |
There was a problem hiding this comment.
The mock auth server ignores JSON decode errors for the login request body. If the client sends invalid JSON, the test will silently treat it as bad credentials instead of failing with a clearer 400 response. Checking the decode error (and returning StatusBadRequest) would make the e2e tests more diagnostic.
| json.NewDecoder(r.Body).Decode(&body) | |
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | |
| w.WriteHeader(http.StatusBadRequest) | |
| json.NewEncoder(w).Encode(map[string]string{"error": "invalid request body"}) | |
| return | |
| } |
- Handle EOF without newline for piped stdin (username & password) - Add newUnauthenticatedClient() to suppress token warnings during login - Validate CurrentContext before API call with clear error message - Change whoami --quiet to output user ID (matches global --quiet contract) - Integration test now loads persisted token back and verifies with fresh client - E2E mock server validates JSON decode errors (returns 400)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil && err != io.EOF { | ||
| return fmt.Errorf("reading password: %w", err) | ||
| } | ||
| password = strings.TrimSpace(line) |
There was a problem hiding this comment.
Password input read from non-terminal stdin is passed through strings.TrimSpace, which can silently change a password that contains leading/trailing spaces. Consider trimming only line endings (e.g., \r/\n) instead of all whitespace so the password value is preserved exactly.
| password = strings.TrimSpace(line) | |
| password = strings.TrimRight(line, "\r\n") |
| fmt.Fprint(cmd.OutOrStdout(), "Username: ") | ||
| reader := bufio.NewReader(cmd.InOrStdin()) | ||
| line, err := reader.ReadString('\n') | ||
| if err != nil && err != io.EOF { | ||
| return fmt.Errorf("reading username: %w", err) | ||
| } | ||
| username = strings.TrimSpace(line) | ||
| } | ||
| if username == "" { | ||
| return fmt.Errorf("username is required") | ||
| } | ||
|
|
||
| if password == "" { | ||
| fmt.Fprint(cmd.OutOrStdout(), "Password: ") | ||
| if f, ok := cmd.InOrStdin().(*os.File); ok && term.IsTerminal(int(f.Fd())) { | ||
| raw, err := term.ReadPassword(int(f.Fd())) | ||
| fmt.Fprintln(cmd.OutOrStdout()) // newline after hidden input | ||
| if err != nil { |
There was a problem hiding this comment.
Interactive prompts ("Username:", "Password:") are currently written to stdout via cmd.OutOrStdout(). This can pollute stdout in non-interactive usage (pipes/scripts). Prefer writing prompts/newlines to cmd.ErrOrStderr() while keeping command output on stdout.
| // Insecure: flag > config | ||
| insecure := flagInsecure | ||
| if !insecure && cfg.CurrentCtx() != nil { | ||
| insecure = cfg.CurrentCtx().Insecure | ||
| } | ||
| if insecure { | ||
| if t, ok := http.DefaultTransport.(*http.Transport); ok { | ||
| clone := t.Clone() | ||
| clone.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // user-requested | ||
| c.HTTPClient.Transport = clone | ||
| } else { | ||
| c.HTTPClient.Transport = &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // user-requested | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The TLS "insecure" transport setup logic is duplicated between newClient() and newUnauthenticatedClient(). This duplication risks the two paths drifting (e.g., future proxy/timeout changes). Consider extracting a shared helper to apply insecure TLS settings to a client.
| tokenData, _ := json.Marshal(map[string]interface{}{ | ||
| "token": resp.Token, | ||
| "expires_at": expiresAt, | ||
| "username": resp.User.Username, | ||
| }) |
There was a problem hiding this comment.
json.Marshal return value is ignored here. While unlikely, a marshal failure would be silently swallowed and could make the test pass/fail for the wrong reason. Please assert/require the marshal error is nil.
| tokenData, _ := json.Marshal(map[string]interface{}{ | |
| "token": resp.Token, | |
| "expires_at": expiresAt, | |
| "username": resp.User.Username, | |
| }) | |
| tokenData, err := json.Marshal(map[string]interface{}{ | |
| "token": resp.Token, | |
| "expires_at": expiresAt, | |
| "username": resp.User.Username, | |
| }) | |
| require.NoError(t, err) |
| tokenData, _ := json.Marshal(map[string]interface{}{ | ||
| "token": "expired-jwt", | ||
| "expires_at": time.Now().Add(-1 * time.Hour).UTC().Format(time.RFC3339), | ||
| "username": "admin", | ||
| }) |
There was a problem hiding this comment.
json.Marshal return value is ignored here. Please assert/require the marshal error is nil so the test fails clearly if marshaling ever breaks.
| tokenData, _ := json.Marshal(map[string]interface{}{ | |
| "token": "expired-jwt", | |
| "expires_at": time.Now().Add(-1 * time.Hour).UTC().Format(time.RFC3339), | |
| "username": "admin", | |
| }) | |
| tokenData, err := json.Marshal(map[string]interface{}{ | |
| "token": "expired-jwt", | |
| "expires_at": time.Now().Add(-1 * time.Hour).UTC().Format(time.RFC3339), | |
| "username": "admin", | |
| }) | |
| require.NoError(t, err) |
| func TestAuthWorkflow_LoginResponseExpiryParsing(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test in short mode") | ||
| } | ||
|
|
There was a problem hiding this comment.
This test is named as if it validates expiry parsing, but the client Login() method currently treats ExpiresAt as an opaque string and does not parse it, and wantErr is always false. Either rename the test to reflect what it actually asserts (decoding/passthrough), or add a unit that exercises the real expiry parsing logic (which appears to live in the cmd layer).
- Only trim \r\n from password input, not all whitespace (preserves spaces) - Move interactive prompts to stderr to avoid polluting piped stdout - Extract applyInsecureTLS() helper to deduplicate TLS config logic - Assert json.Marshal errors in integration tests (2 instances) - Rename ExpiryParsing test to ExpiryPassthrough (matches actual behavior)
Plugin code fixes: - discoverPlugins now resolves binaryPath to an absolute path via filepath.Abs, so relative $PATH entries can't leave a relative path stored as the plugin's captured exec target (Copilot #1). - Validate plugin name against [a-z0-9][a-z0-9-]* to reject filenames with whitespace or leading dashes (stackctl--help etc.) that would break Cobra routing (Copilot #12). - Windows portability: skip the POSIX executable-bit check on GOOS windows and trim .exe from the plugin name so stackctl-foo.exe becomes `stackctl foo` the same way it does on Unix (Copilot #7). Output registry fixes: - Normalise format names via strings.ToLower + TrimSpace in RegisterFormat / RegisterSingleFormat / lookups and the built-in collision check. Previously RegisterFormat("CSV", ...) was unreachable because NewPrinter lowercases `-o csv`; now case and leading/trailing whitespace don't matter (Copilot #5). Added 2 tests covering the new contract: `RegisterFormat(" CSV ", fn)` is reached by `-o csv`, and `RegisterFormat("JSON", ...)` panics the same way as "json". - Reword the "racy" comment — the registry IS synchronised, just not safe semantically (inconsistent renders across concurrent calls). Previous wording implied a data race (Copilot #4). Doc fixes (stale help output + API_URL semantics): - README.md and EXTENDING.md: example help output no longer shows the plugin's absolute path — matches current Short string (Copilot #2, #8). - EXTENDING.md "internals" section now documents that Short is short and Long carries the resolved path (Copilot #10). - EXTENDING.md env-vars section clarifies that STACKCTL_API_URL / STACKCTL_API_KEY are inherited from the parent shell ONLY — stackctl does not resolve them from ~/.stackmanager/config.yaml when execing plugins (Copilot #9, #11). Troubleshooting section updated with two recommended workflows: explicit export, or plugin-side config parse. - EXTENDING.md arguments section fixes `--help <name>` (not a real Cobra invocation) to `help <name>` (Copilot #3). Test convention fix: - cli/cmd/plugins_test.go: remove all t.Parallel() calls per project convention — cmd/ tests mutate package-level globals and must run serially. The .github/instructions/tests.instructions.md file explicitly documents this (Copilot #6). All 6 stackctl test packages pass. go vet clean.
* feat(plugin): external command discovery (git/kubectl/gh style) On Execute(), scan $PATH for executables named stackctl-<name> and register each as a top-level subcommand that proxies invocation to the external binary. First-path-wins semantics; built-in subcommands always win on name collision; non-regular files, directories, and non-executables are skipped; stdin/stdout/stderr and exit codes pass through. This unblocks third-party extensions without requiring stackctl itself to depend on any plugin SDK -- the only contract is the naming convention and environment-variable inheritance (STACKCTL_API_URL, STACKCTL_API_KEY, etc. are inherited naturally via os.Environ()). Tests (9 subtests) cover: discovery of valid plugins, skip non-executables, skip empty/missing $PATH entries, first-wins with duplicates, subcommand registration, built-in collision protection, argument passthrough, no-op on empty PATH, Cobra routing via RunE. Part of the broader extensibility refactor -- the Klaravik-specific refresh-db subcommand will move into a stackctl-refresh-db external plugin in a follow-up cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(output): pluggable format registry in pkg/output Users can now add custom output formats via RegisterFormat(name, fn) alongside the built-in table/json/yaml. A matching RegisterSingleFormat adds custom single-item rendering; when absent, single items adapt to the list formatter via a 2-column (field, value) table. Built-in format names cannot be shadowed (panics to surface the mistake at init). Unknown format names fall back to table, matching existing behaviour for users who misspell --output flags. Mirrors the extension pattern the backend uses for template funcs: global registry, register at init time, no mutation during concurrent render. Tests (8) cover: panic on built-in override, NewPrinter recognises custom format, unknown format falls back to table, list formatter receives data and headers/rows, single-item fallback adapts to list formatter, custom single formatter wins when registered, concurrent reads are race-free. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(cli)!: remove `stack refresh-db` — migrated to external plugin Parity has been verified end-to-end in the dev cluster (commit 4d82515 on k8s-stack-manager removed the corresponding backend endpoint). The refresh-db operation now lives entirely in the stackctl-refresh-db plugin shipped from kvk-devops-tools, discovered via the PATH-scanning plugin mechanism added in 10953bf. Removed: - stackRefreshDBCmd in cmd/stack.go (definition + flag + subcommand registration) - Client.RefreshDBStack in pkg/client/client.go - TestStackRefreshDBCmd_* (4 tests) in cmd/stack_test.go - TestRefreshDBStack_Success + _Conflict in pkg/client/client_test.go BREAKING CHANGE: `stackctl stack refresh-db <id>` no longer exists. Install the stackctl-refresh-db plugin from kvk-devops-tools/refresh-db/ onto your PATH; `stackctl refresh-db <id>` then becomes available automatically via external-command discovery. The top-level name changed too (refresh-db is no longer a stack-scoped subcommand). Full test suite green across all 6 packages. * docs(plugins): add EXTENDING.md guide for stackctl plugin authors Promotes external-command discovery (10953bf) as a headline feature with tutorial-first documentation. - EXTENDING.md (repo root, ~280 lines): 5-minute tutorial (stackctl-hello in 4 lines of bash), use-case guidance, naming convention, environment inheritance, three language recipes (bash / Python / Go) for the most common pattern (invoke a server-side action), best practices, plugin distribution strategies, internals overview, troubleshooting. - README.md: "Extending — add your own subcommands" section right after Quick Start, with a copy-paste example and a link to EXTENDING.md. Cross-links to k8s-stack-manager's EXTENDING.md — the plugin side and the webhook side together form the complete end-to-end story for adding custom operations without forking either tool. * chore(plugin): address code-review feedback - Forward --insecure, --quiet, --output into plugin env as STACKCTL_INSECURE, STACKCTL_QUIET, STACKCTL_OUTPUT so plugins see the same effective config stackctl uses for built-in commands. EXTENDING.md advertised this contract but the code didn't deliver it. - Hide the absolute plugin path from --help (was leaking $HOME in screenshots). Full path still visible in `stackctl help <plugin>` Long text for debugging. - Reject nil FormatterFunc / SingleFormatterFunc at RegisterFormat time so misuse surfaces at init, not inside a render path minutes later. - Strengthen plugin collision test: assert exactly one command named "config" exists (not two with Cobra silently accepting the dupe). - Add stdin-passthrough test using a cat-style plugin — the production code already routes stdin via cmd.InOrStdin() but that contract was previously untested. - Polish Python recipe in EXTENDING.md: proper HTTPError/URLError handling + stderr warning when STACKCTL_INSECURE is active. - Expand the "Environment variables inherited" section with the full resolved-from-flags table and a security note that the entire parent environment (including unrelated creds) is forwarded to every plugin. All 6 test packages green. * chore(plugin): address Copilot review feedback on PR #35 Plugin code fixes: - discoverPlugins now resolves binaryPath to an absolute path via filepath.Abs, so relative $PATH entries can't leave a relative path stored as the plugin's captured exec target (Copilot #1). - Validate plugin name against [a-z0-9][a-z0-9-]* to reject filenames with whitespace or leading dashes (stackctl--help etc.) that would break Cobra routing (Copilot #12). - Windows portability: skip the POSIX executable-bit check on GOOS windows and trim .exe from the plugin name so stackctl-foo.exe becomes `stackctl foo` the same way it does on Unix (Copilot #7). Output registry fixes: - Normalise format names via strings.ToLower + TrimSpace in RegisterFormat / RegisterSingleFormat / lookups and the built-in collision check. Previously RegisterFormat("CSV", ...) was unreachable because NewPrinter lowercases `-o csv`; now case and leading/trailing whitespace don't matter (Copilot #5). Added 2 tests covering the new contract: `RegisterFormat(" CSV ", fn)` is reached by `-o csv`, and `RegisterFormat("JSON", ...)` panics the same way as "json". - Reword the "racy" comment — the registry IS synchronised, just not safe semantically (inconsistent renders across concurrent calls). Previous wording implied a data race (Copilot #4). Doc fixes (stale help output + API_URL semantics): - README.md and EXTENDING.md: example help output no longer shows the plugin's absolute path — matches current Short string (Copilot #2, #8). - EXTENDING.md "internals" section now documents that Short is short and Long carries the resolved path (Copilot #10). - EXTENDING.md env-vars section clarifies that STACKCTL_API_URL / STACKCTL_API_KEY are inherited from the parent shell ONLY — stackctl does not resolve them from ~/.stackmanager/config.yaml when execing plugins (Copilot #9, #11). Troubleshooting section updated with two recommended workflows: explicit export, or plugin-side config parse. - EXTENDING.md arguments section fixes `--help <name>` (not a real Cobra invocation) to `help <name>` (Copilot #3). Test convention fix: - cli/cmd/plugins_test.go: remove all t.Parallel() calls per project convention — cmd/ tests mutate package-level globals and must run serially. The .github/instructions/tests.instructions.md file explicitly documents this (Copilot #6). All 6 stackctl test packages pass. go vet clean. * fix(plugin): address Copilot review round 2 - EXTENDING.md: remove credential leak in tutorial bash snippet (the VAR-prefix-star pattern echoed the real API key when set) - EXTENDING.md: clarify that plugins do not inherit config-file auth, document env-var and stackctl-config-show strategies - EXTENDING.md: frame /actions/ as a k8s-stack-manager (external) endpoint, not a stackctl feature - output: NewPrinter uses the shared normalizeFormatName (TrimSpace+ToLower) so whitespace in the flag no longer mismatches the registry key - output: RegisterSingleFormat panics if base format not registered, catches silent wiring mistakes - plugins: flag-vs-env precedence honours an explicit --flag=false via PersistentFlags().Changed - plugins: clarify that only .exe is trimmed on Windows (not full PATHEXT) - plugins: align pluginNamePattern comment with the actual regex (lowercase) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(plugin): address Copilot review round 3 - EXTENDING.md: fix Short description to match implementation (no path in Short) - EXTENDING.md: replace nonexistent stackctl config show with config get - EXTENDING.md: replace config-file parsing advice with config get commands - EXTENDING.md: fix bash error hint to suggest export instead of config set - README.md: clarify that plugins need explicitly exported env vars - output.go: RegisterFormat rejects empty/whitespace-only names Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(plugin): address Copilot review round 4 - output: RegisterSingleFormat rejects empty/whitespace-only names (matches RegisterFormat) - plugins: normalize STACKCTL_OUTPUT value before passing to subprocess - formatter_test: use unique format key to avoid parallel-test registry collision Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(plugin): address Copilot review round 5 - Prevent help/completion shadowing by external plugins - Case-insensitive .exe suffix stripping on Windows Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Olof Mattsson <olof.mattsson@klaravik.se> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Implements authentication commands for stackctl (closes #2).
Commands Added
stackctl login— Authenticate via username/password (interactive, flags, or env vars)stackctl logout— Clear stored JWT for current contextstackctl whoami— Display current user info (table/json/yaml/quiet)Changes
pkg/types/types.goLoginRequest,LoginResponsetypespkg/client/client.goLogin()andWhoami()client methodscmd/login.gocmd/login_test.gopkg/client/client_test.gotest/integration/auth_integration_test.gotest/e2e/cli_e2e_test.goFeatures
golang.org/x/term(terminal masking)STACKCTL_USERNAME/STACKCTL_PASSWORDenv vars for CI/CD0600permissionswhoamiTesting