From 9c405e7223922c6e06ee087dd37696f325a775cc Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Fri, 31 Jul 2026 09:59:21 +1000 Subject: [PATCH 1/2] fix: KEEP-1049 compare CLI version against the server's advertised floor kh doctor's CLI Version check only ever printed the local version and unconditionally passed - the server now advertises a floor via KH-Minimum-CLI-Version (KEEP-1047), but nothing compared against it. checkCLIVersion probes /api/health for the header and reuses khhttp.SemverLessThan to warn, naming the remedy, when the local build is behind. --- cmd/doctor/doctor.go | 41 ++++++++++-- cmd/doctor/doctor_version_test.go | 106 ++++++++++++++++++++++++++++++ internal/http/version.go | 7 +- 3 files changed, 149 insertions(+), 5 deletions(-) create mode 100644 cmd/doctor/doctor_version_test.go diff --git a/cmd/doctor/doctor.go b/cmd/doctor/doctor.go index be6f2a0..924d4e9 100644 --- a/cmd/doctor/doctor.go +++ b/cmd/doctor/doctor.go @@ -368,14 +368,47 @@ func checkChains(ctx context.Context, f *cmdutil.Factory) CheckResult { return CheckResult{Status: "pass", Message: fmt.Sprintf("%d chains available", len(chains))} } -// checkCLIVersion reports the current CLI version. Network-based latest-version -// checking is deferred to Phase 24; this check is always instantaneous. -func checkCLIVersion(_ context.Context, f *cmdutil.Factory) CheckResult { +// checkCLIVersion reports the current CLI version, and warns if it is older +// than the floor the server advertises via the KH-Minimum-CLI-Version +// response header (see khhttp.MinimumVersionHeader). +func checkCLIVersion(ctx context.Context, f *cmdutil.Factory) CheckResult { v := f.AppVersion if v == "" || v == "dev" || strings.HasPrefix(v, "v0.0.0") { return CheckResult{Status: "warn", Message: "development build"} } - return CheckResult{Status: "pass", Message: fmt.Sprintf("v%s", strings.TrimPrefix(v, "v"))} + current := strings.TrimPrefix(v, "v") + localVersion := "v" + current + + host, err := getHost(f) + if err != nil { + return CheckResult{Status: "pass", Message: localVersion} + } + client, err := getHTTPClient(f) + if err != nil { + return CheckResult{Status: "pass", Message: localVersion} + } + + url := khhttp.BuildBaseURL(host) + "/api/health" + resp, err := doGet(ctx, client, host, url) + if err != nil { + // No server to compare against -- report the local version rather + // than failing a check whose job is reachability elsewhere (checkAPI). + return CheckResult{Status: "pass", Message: localVersion} + } + defer resp.Body.Close() + _, _ = io.Copy(io.Discard, resp.Body) + + minimum := resp.Header.Get(khhttp.MinimumVersionHeader) + if minimum == "" { + return CheckResult{Status: "pass", Message: localVersion} + } + if khhttp.SemverLessThan(current, minimum) { + return CheckResult{ + Status: "warn", + Message: fmt.Sprintf("%s is outdated; minimum required is %s. Run: kh update", localVersion, minimum), + } + } + return CheckResult{Status: "pass", Message: localVersion} } // abbreviateAddr shortens a long address like 0xABCD...1234. diff --git a/cmd/doctor/doctor_version_test.go b/cmd/doctor/doctor_version_test.go new file mode 100644 index 0000000..4b5e6eb --- /dev/null +++ b/cmd/doctor/doctor_version_test.go @@ -0,0 +1,106 @@ +package doctor_test + +// checkCLIVersion used to only print the local version string, unconditionally +// pass, and never compare against anything -- the server had no way to tell an +// out-of-date CLI that it was behind. These tests pin the comparison against +// the KH-Minimum-CLI-Version header the server advertises on /api/* +// (khhttp.MinimumVersionHeader). + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/keeperhub/cli/cmd/doctor" + "github.com/keeperhub/cli/internal/config" + khhttp "github.com/keeperhub/cli/internal/http" + "github.com/keeperhub/cli/pkg/cmdutil" + "github.com/keeperhub/cli/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// cliVersionLine returns the doctor output line reporting the CLI Version check. +func cliVersionLine(out string) string { + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "CLI Version:") { + return line + } + } + return "" +} + +// versionDoctorFactory builds a Factory pinned to appVersion, backed by a test +// server that advertises minimumHeader (empty means the header is omitted). +func versionDoctorFactory(ios *iostreams.IOStreams, appVersion, minimumHeader string) *cmdutil.Factory { + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if minimumHeader != "" && strings.HasPrefix(r.URL.Path, "/api/") { + w.Header().Set(khhttp.MinimumVersionHeader, minimumHeader) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + return &cmdutil.Factory{ + AppVersion: appVersion, + IOStreams: ios, + Config: func() (config.Config, error) { + return config.Config{DefaultHost: svr.URL}, nil + }, + HTTPClient: func() (*khhttp.Client, error) { + return khhttp.NewClient(khhttp.ClientOptions{ + AppVersion: appVersion, + IOStreams: ios, + }), nil + }, + } +} + +func TestDoctorCmd_CLIVersionPassesWhenServerAdvertisesNoMinimum(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + ios, outBuf, _, _ := iostreams.Test() + tc := doctor.NewTestableCmd(versionDoctorFactory(ios, "1.2.3", "")) + require.NoError(t, tc.Execute([]string{})) + + line := cliVersionLine(outBuf.String()) + assert.Contains(t, line, "pass") + assert.Contains(t, line, "v1.2.3") +} + +func TestDoctorCmd_CLIVersionWarnsWhenBelowServerMinimum(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + ios, outBuf, _, _ := iostreams.Test() + tc := doctor.NewTestableCmd(versionDoctorFactory(ios, "0.3.0", "0.11.1")) + require.NoError(t, tc.Execute([]string{})) + + line := cliVersionLine(outBuf.String()) + assert.Contains(t, line, "warn") + assert.Contains(t, line, "outdated") + assert.Contains(t, line, "0.11.1") + assert.Contains(t, line, "Run: kh update") +} + +func TestDoctorCmd_CLIVersionPassesWhenAtServerMinimum(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + ios, outBuf, _, _ := iostreams.Test() + tc := doctor.NewTestableCmd(versionDoctorFactory(ios, "0.11.1", "0.11.1")) + require.NoError(t, tc.Execute([]string{})) + + line := cliVersionLine(outBuf.String()) + assert.Contains(t, line, "pass") + assert.NotContains(t, line, "outdated") +} + +func TestDoctorCmd_CLIVersionReportsDevBuildWithoutComparing(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + ios, outBuf, _, _ := iostreams.Test() + // A minimum far above any real release: if dev builds were compared, + // this would incorrectly warn. + tc := doctor.NewTestableCmd(versionDoctorFactory(ios, "dev", "99.0.0")) + require.NoError(t, tc.Execute([]string{})) + + line := cliVersionLine(outBuf.String()) + assert.Contains(t, line, "development build") + assert.NotContains(t, line, "outdated") +} diff --git a/internal/http/version.go b/internal/http/version.go index 2b1de7e..1f9d4a5 100644 --- a/internal/http/version.go +++ b/internal/http/version.go @@ -8,6 +8,11 @@ import ( "strings" ) +// MinimumVersionHeader is the response header the server uses to advertise +// the oldest CLI version it considers supported (see keeperhub's +// lib/cli-version.ts). Shared so every call site checks the same header. +const MinimumVersionHeader = "KH-Minimum-CLI-Version" + // semverLessThan returns true if current is strictly less than minimum. // Returns false if current is "dev" or unparseable -- dev builds never trigger warnings. func semverLessThan(current, minimum string) bool { @@ -62,7 +67,7 @@ func checkVersion(current string, resp *http.Response, errOut io.Writer) { if resp == nil { return } - minimum := resp.Header.Get("KH-Minimum-CLI-Version") + minimum := resp.Header.Get(MinimumVersionHeader) if minimum == "" { return } From d4f1111cd625c25d4a7f612d119434a8f8cd0950 Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Fri, 31 Jul 2026 09:59:31 +1000 Subject: [PATCH 2/2] fix: KEEP-1049 stop presenting the API-key prefix as an identity fetchAPIKeyInfo has no real identity for an API key, so it sets Email to a truncated key prefix (kh_EU7Fc1Xi...). kh auth status and kh auth login printed that prefix under "User" / "logged in as", which reads as though the key were the account. Both now name it as a credential for API-key auth while leaving session auth unchanged. --- cmd/auth/login.go | 15 ++++++-- cmd/auth/login_apikey_test.go | 31 +++++++++++++++++ cmd/auth/status.go | 11 +++++- cmd/auth/status_test.go | 64 +++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 382731c..aa0d4eb 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -35,6 +35,17 @@ var FetchTokenInfoFunc = func(host, token string) (internalauth.TokenInfo, error return internalauth.FetchTokenInfo(host, token) } +// credentialIdentity describes info for "logged in as %s" style messages. +// API keys have no real identity to show - info.Email holds a truncated key +// prefix for lack of one - so it is named as a credential rather than +// presented as though the key were an account. +func credentialIdentity(info internalauth.TokenInfo) string { + if info.Method == internalauth.AuthMethodAPIKey { + return "API key " + info.Email + } + return info.Email +} + func NewLoginCmd(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "login", @@ -78,7 +89,7 @@ See also: kh auth status, kh auth logout`, if info, infoErr := FetchTokenInfoFunc(host, entry.Token); infoErr == nil { fmt.Fprintf(f.IOStreams.Out, "Already logged in to %s as %s\nUse --force to replace the stored API key.\n", - host, info.Email) + host, credentialIdentity(info)) return nil } } @@ -111,7 +122,7 @@ See also: kh auth status, kh auth logout`, if err != nil { fmt.Fprintf(f.IOStreams.Out, "Logged in to %s\n", host) } else { - fmt.Fprintf(f.IOStreams.Out, "Logged in to %s as %s\n", host, info.Email) + fmt.Fprintf(f.IOStreams.Out, "Logged in to %s as %s\n", host, credentialIdentity(info)) } // The device flow creates a real organization API key rather than a diff --git a/cmd/auth/login_apikey_test.go b/cmd/auth/login_apikey_test.go index 756d542..b29a788 100644 --- a/cmd/auth/login_apikey_test.go +++ b/cmd/auth/login_apikey_test.go @@ -56,6 +56,37 @@ func TestLoginCmd_SkipsWhenStoredTokenIsValid(t *testing.T) { } } +// fetchAPIKeyInfo sets Email to a truncated key prefix for lack of a real +// identity. This pins that the already-logged-in notice names it as a +// credential rather than presenting the prefix as though it were an account. +func TestLoginCmd_AlreadyLoggedInNamesAPIKeyAsCredential(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + if err := config.SetHostToken(testHost, "kh_already_stored"); err != nil { + t.Fatalf("seeding token: %v", err) + } + + ios, buf, _, _ := iostreams.Test() + auth.DeviceLoginFunc = func(string, *iostreams.IOStreams) (string, error) { + t.Fatal("DeviceLogin must not run when a valid token is stored") + return "", nil + } + auth.SetTokenFunc = func(string, string) error { return nil } + auth.FetchTokenInfoFunc = func(_, token string) (internalauth.TokenInfo, error) { + return internalauth.TokenInfo{Email: "kh_EU7Fc1Xi...", Method: internalauth.AuthMethodAPIKey}, nil + } + + cmd := auth.NewLoginCmd(&cmdutil.Factory{IOStreams: ios}) + cmd.SetArgs([]string{}) + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "as API key kh_EU7Fc1Xi...") { + t.Errorf("expected the key prefix named as an API key, got: %q", out) + } +} + func TestLoginCmd_ForceReplacesStoredToken(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) if err := config.SetHostToken(testHost, "kh_already_stored"); err != nil { diff --git a/cmd/auth/status.go b/cmd/auth/status.go index 8aedfae..79295f0 100644 --- a/cmd/auth/status.go +++ b/cmd/auth/status.go @@ -85,10 +85,19 @@ func NewStatusCmd(f *cmdutil.Factory) *cobra.Command { Host: host, } + // API keys have no real identity to show - info.Email holds a + // truncated key prefix for lack of one - so the row is labeled as + // a credential rather than presented as though the key were a + // user account. + userLabel := "User" + if info.Method == internalauth.AuthMethodAPIKey { + userLabel = "Credential" + } + p := output.NewPrinter(f.IOStreams, cmd) return p.PrintData(data, func(tw table.Writer) { tw.AppendRow(table.Row{"Host", host}) - tw.AppendRow(table.Row{"User", info.Email}) + tw.AppendRow(table.Row{userLabel, info.Email}) tw.AppendRow(table.Row{"Organization", info.OrgName}) tw.AppendRow(table.Row{"Role", info.Role}) if expiresAt != "" { diff --git a/cmd/auth/status_test.go b/cmd/auth/status_test.go index 7537c62..bfde20e 100644 --- a/cmd/auth/status_test.go +++ b/cmd/auth/status_test.go @@ -97,6 +97,70 @@ func TestStatusCmd_APIKeyMethod(t *testing.T) { } } +// fetchAPIKeyInfo has no real identity for an API key, so it sets Email to a +// truncated key prefix (e.g. "kh_EU7Fc1Xi..."). This pins that the table +// stops labeling that prefix "User", which reads as though the key were an +// account, and uses "Credential" for API-key auth instead. +func TestStatusCmd_APIKeyLabeledAsCredentialNotUser(t *testing.T) { + ios, buf, _, _ := iostreams.Test() + + auth.ResolveTokenFunc = func(host string) (internalauth.ResolvedToken, error) { + return internalauth.ResolvedToken{Token: "kh_EU7Fc1Xi", Method: internalauth.AuthMethodAPIKey, Host: host}, nil + } + auth.FetchTokenInfoFunc = func(host, token string) (internalauth.TokenInfo, error) { + return internalauth.TokenInfo{ + Email: "kh_EU7Fc1Xi...", + OrgName: "My Org", + Role: "api-key", + Method: internalauth.AuthMethodAPIKey, + }, nil + } + + f := &cmdutil.Factory{IOStreams: ios} + cmd := auth.NewStatusCmd(f) + cmd.SetArgs([]string{}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "Credential") { + t.Errorf("expected the row labeled Credential for API-key auth, got: %q", out) + } + if strings.Contains(out, "User") { + t.Errorf("must not label the key prefix as User, got: %q", out) + } +} + +func TestStatusCmd_SessionLabeledAsUser(t *testing.T) { + ios, buf, _, _ := iostreams.Test() + + auth.ResolveTokenFunc = func(host string) (internalauth.ResolvedToken, error) { + return internalauth.ResolvedToken{Token: "tok", Method: internalauth.AuthMethodToken, Host: host}, nil + } + auth.FetchTokenInfoFunc = func(host, token string) (internalauth.TokenInfo, error) { + return internalauth.TokenInfo{ + Email: "user@example.com", + OrgName: "My Org", + Role: "owner", + Method: internalauth.AuthMethodToken, + }, nil + } + + f := &cmdutil.Factory{IOStreams: ios} + cmd := auth.NewStatusCmd(f) + cmd.SetArgs([]string{}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !strings.Contains(buf.String(), "User") { + t.Errorf("expected the row labeled User for session auth, got: %q", buf.String()) + } +} + func TestStatusCmd_JSONOutput(t *testing.T) { ios, buf, _, _ := iostreams.Test()