From bb69cbc479e2872670cd5cffc7e6de00da1d96ba Mon Sep 17 00:00:00 2001 From: Andreas Grub Date: Mon, 2 Nov 2026 14:08:45 +0100 Subject: [PATCH 1/5] feat: show the status of an API key from the stored files only meshStack removes the endpoint that let an API key read itself. The status of a credential now comes from the stored credential and the claims of its cached token, so it makes no network call. For a new API key it shows that no token is cached yet. ReadSelf and the permission grouping that only the status used are removed. Co-Authored-By: Claude Opus 5.5 --- client/api_key.go | 7 -- client/api_key_permissions.go | 33 -------- client/api_key_permissions_test.go | 13 ---- cmd/auth/status.go | 2 +- cmd/internal/markdown/credential.md.tmpl | 12 --- cmd/internal/markdown/markdown_test.go | 40 +--------- cmd/internal/testacc/api_get_test.go | 10 +-- cmd/internal/testacc/login_test.go | 6 +- cmd/profile/model.go | 2 +- cmd/profile/show.go | 18 +---- cmd/profile/show.md.tmpl | 2 +- cmd/profile/show_test.go | 32 -------- docs/profile.md | 5 +- internal/auth/status.go | 79 ++----------------- internal/auth/status_test.go | 99 +++++------------------- 15 files changed, 35 insertions(+), 325 deletions(-) diff --git a/client/api_key.go b/client/api_key.go index 99275cfc..6605a449 100644 --- a/client/api_key.go +++ b/client/api_key.go @@ -36,9 +36,6 @@ type MeshApiKeyStatus struct { type MeshApiKeyClient interface { Create(ctx context.Context, apiKey *MeshApiKey) (*MeshApiKey, error) Read(ctx context.Context, uuid string) (*MeshApiKey, error) - // ReadSelf returns the key the request authenticates with. Unlike Read, it needs no APIKEY_LIST - // and also finds a hidden key, such as one bootstrapped from the meshStack configuration. - ReadSelf(ctx context.Context) (*MeshApiKey, error) Update(ctx context.Context, uuid string, apiKey *MeshApiKey) (*MeshApiKey, error) Delete(ctx context.Context, uuid string) error } @@ -63,10 +60,6 @@ func (c meshApiKeyClient) Update(ctx context.Context, uuid string, apiKey *MeshA return c.meshObject.Put(ctx, uuid, apiKey) } -func (c meshApiKeyClient) ReadSelf(ctx context.Context) (*MeshApiKey, error) { - return c.meshObject.Get(ctx, "self") -} - func (c meshApiKeyClient) Delete(ctx context.Context, uuid string) error { return c.meshObject.Delete(ctx, uuid) } diff --git a/client/api_key_permissions.go b/client/api_key_permissions.go index d456c864..800c6a1e 100644 --- a/client/api_key_permissions.go +++ b/client/api_key_permissions.go @@ -43,39 +43,6 @@ func (p ApiKeyPermissions) WorkspaceCodes() []string { return codes } -// Only keeps the codes of p that are among codes, in the order of p, and puts the codes p does -// not know into a last group named Other. -func (p ApiKeyPermissions) Only(codes []ApiPermission) ApiKeyPermissions { - remaining := slices.Clone(codes) - var only ApiKeyPermissions - for _, group := range p { - kept := ApiKeyPermissionGroup{Name: group.Name} - for _, suffixGroup := range group.Actions { - var held []ApiPermission - for _, code := range suffixGroup { - if index := slices.Index(remaining, code); index >= 0 { - remaining = slices.Delete(remaining, index, index+1) - held = append(held, code) - } - } - if len(held) > 0 { - kept.Actions = append(kept.Actions, held) - } - } - if len(kept.Actions) > 0 { - only = append(only, kept) - } - } - if len(remaining) > 0 { - other := ApiKeyPermissionGroup{Name: "Other"} - for _, code := range remaining { - other.Actions = append(other.Actions, []ApiPermission{code}) - } - only = append(only, other) - } - return only -} - func (p ApiKeyPermissions) MarkdownString() string { var lines []string for _, group := range p { diff --git a/client/api_key_permissions_test.go b/client/api_key_permissions_test.go index 55b2a6cd..f3d1ed75 100644 --- a/client/api_key_permissions_test.go +++ b/client/api_key_permissions_test.go @@ -23,19 +23,6 @@ func TestPermissions(t *testing.T) { } }) - t.Run("Only keeps the held codes in their groups, and the unknown ones in Other", func(t *testing.T) { - held := Permissions.Only([]ApiPermission{ - "WORKSPACE_LIST", "NOT_A_KNOWN_CODE", "TENANT_DELETE", "ADM_TENANT_IMPORT", "MANAGED_TENANT_IMPORT", "ADM_TENANT_DELETE", - }) - - assert.Equal(t, ApiKeyPermissions{ - {Name: "Tenants", Actions: [][]ApiPermission{{"TENANT_DELETE", "ADM_TENANT_DELETE"}, {"MANAGED_TENANT_IMPORT", "ADM_TENANT_IMPORT"}}}, - {Name: "Workspaces", Actions: [][]ApiPermission{{"WORKSPACE_LIST"}}}, - {Name: "Other", Actions: [][]ApiPermission{{"NOT_A_KNOWN_CODE"}}}, - }, held) - assert.Empty(t, Permissions.Only(nil)) - }) - t.Run("MarkdownString merges the codes into patterns that stand for no other code", func(t *testing.T) { permissions := ApiKeyPermissions{ {Name: "Tenants", Actions: [][]ApiPermission{ diff --git a/cmd/auth/status.go b/cmd/auth/status.go index 5f994eaf..5bbdef9e 100644 --- a/cmd/auth/status.go +++ b/cmd/auth/status.go @@ -44,7 +44,7 @@ func newStatus() *cobra.Command { where it comes from, who it is for, and until when it lasts. It never shows a secret. A browser login is not renewed for this, so its token may show as expired, and it is renewed on -next use. An API key reads its own details from meshStack.`, +next use.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { session, err := auth.ResolveSession(cmd.Context(), internal.ResolveClientOptions()) diff --git a/cmd/internal/markdown/credential.md.tmpl b/cmd/internal/markdown/credential.md.tmpl index 2fd5dfde..25549a05 100644 --- a/cmd/internal/markdown/credential.md.tmpl +++ b/cmd/internal/markdown/credential.md.tmpl @@ -12,12 +12,6 @@ | Credential | {{.Kind.Label}} {{- with .ApiKey}}{{if eq $.Kind "manual"}} of API key{{end}} {{.ClientId}}{{else}}{{if and $token $token.ClientId}} of API key {{cell $token.ClientId}}{{end}}{{end -}} , from {{range $i, $source := .Sources}}{{if $i}}, {{end}}{{cell $source}}{{end}} | -{{- with .ApiKey}}{{with .Details}} -| | {{cell .DisplayName}}, owned by workspace {{cell .OwnedByWorkspace}}, -{{- if not .ExpiresAt.IsZero}} expires {{at .ExpiresAt}} -{{- else if .ExpiresOn}} expires on {{cell .ExpiresOn}} -{{- else}} never expires{{end}} | -{{- end}}{{end}} {{- if and $token $token.User (not .ApiKey)}} | | User {{cell $token.User}}{{with $token.Email}}{{if ne . $token.User}} ({{cell .}}){{end}}{{end}}{{if .OidcLogin}} with {{template "access" .}}{{end}} | {{- else if .OidcLogin}} @@ -29,12 +23,6 @@ | | {{with $token}}{{if .Expired}}Token expired {{at .ExpiresAt}}, {{if eq $.Kind "manual"}}cannot be renewed, run `meshstack login -p {{$.Profile}} --apitoken`{{else if and $.OidcLogin $.OidcLogin.SessionEnded}}cannot be renewed{{else}}renewed on next use{{end}} {{- else}}Token expires {{at .ExpiresAt}}{{end}}{{with .Workspace}}, for workspace {{cell .}}{{end}} {{- else}}No token cached yet{{if ne .Kind "manual"}}, minted on next use{{end}}{{end}} | -{{- with .ApiKey}}{{with .Details}} -{{- range .PermissionGroups}} -| | • {{cell .Name}}: {{range $i, $pattern := .Patterns}}{{if $i}}, {{end}}`{{cell $pattern}}`{{end}} | -{{- else}} -| | No permissions | -{{- end}}{{end}}{{end}} {{- range $i, $unused := .Unused}} | {{if not $i}}Unused credentials{{end}} | • {{.Kind.Label}} {{- with .ClientId}} {{cell .}}{{end}} diff --git a/cmd/internal/markdown/markdown_test.go b/cmd/internal/markdown/markdown_test.go index 8546e8f3..e76bc982 100644 --- a/cmd/internal/markdown/markdown_test.go +++ b/cmd/internal/markdown/markdown_test.go @@ -12,7 +12,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/meshcloud/meshstack-cli/client" "github.com/meshcloud/meshstack-cli/client/types/xurl" "github.com/meshcloud/meshstack-cli/internal/auth" "github.com/meshcloud/meshstack-cli/internal/auth/credential" @@ -47,16 +46,13 @@ func TestABrowserLoginShowsItsUserAccessAndSessionEnd(t *testing.T) { }) } -func TestAnApiKeyShowsItsPermissionsBelowTheCredential(t *testing.T) { +func TestAnApiKeyShowsItsClientIdAndNoCachedTokenYet(t *testing.T) { synctest.Test(t, func(t *testing.T) { clientId := uuid.MustParse("11111111-45bf-42ba-a965-2097b9d0d181") status := auth.CredentialStatus{ Profile: "ci", Endpoint: xurl.MustParsef("https://meshstack.example.com"), Kind: credential.ApiKeyName, Sources: []string{"env MESHSTACK_API_KEY", "env MESHSTACK_API_SECRET"}, - ApiKey: &auth.ApiKeyStatus{ClientId: clientId, Details: &auth.ApiKeyDetails{ - DisplayName: "CI | nightly", OwnedByWorkspace: "ops", - PermissionGroups: client.ApiKeyPermissions{{Name: "Tenants", Actions: [][]client.ApiPermission{{"TENANT_LIST", "ADM_TENANT_LIST"}, {"TENANT_SAVE", "ADM_TENANT_SAVE"}}}}, - }}, + ApiKey: &auth.ApiKeyStatus{ClientId: clientId}, Unused: []auth.UnusedCredential{ {Kind: credential.ApiKeyName, ClientId: "22222222-45bf-42ba-a965-2097b9d0d181"}, {Kind: credential.OidcLoginName, Selected: true, Token: &auth.TokenStatus{User: "jane", Email: "jane@example.com"}}, @@ -72,9 +68,7 @@ func TestAnApiKeyShowsItsPermissionsBelowTheCredential(t *testing.T) { "| Endpoint | https://meshstack.example.com |\n"+ "| Workspace | none |\n"+ "| Credential | API key 11111111-45bf-42ba-a965-2097b9d0d181, from env MESHSTACK_API_KEY, env MESHSTACK_API_SECRET |\n"+ - `| | CI \| nightly, owned by workspace ops, never expires |`+"\n"+ "| | No token cached yet, minted on next use |\n"+ - "| | • Tenants: `[ADM_]TENANT_(LIST\\|SAVE)` |\n"+ "| Unused credentials | • API key 22222222-45bf-42ba-a965-2097b9d0d181 |\n"+ "| | • Browser login jane (jane@example.com), selected by the profile |\n"+ "| | • API token, expired 1999-12-31 23:00 (1h ago) |", shown) @@ -96,36 +90,6 @@ func TestAnExpiredApiTokenSaysItCannotBeRenewed(t *testing.T) { assert.NotContains(t, shown, "| Workspace |", "the token carries its workspace") } -func TestAnApiKeyShowsWhenItExpires(t *testing.T) { - for _, tt := range []struct { - name string - expiresIn time.Duration - expiresOn string - want string - }{ - {name: "a key with an expiry time", expiresIn: 30 * 24 * time.Hour, want: "expires 2000-01-31 00:00 (in 30d)"}, - {name: "a meshStack that sends only the expiry date", expiresOn: "2000-01-31", want: "expires on 2000-01-31"}, - } { - t.Run(tt.name, func(t *testing.T) { - synctest.Test(t, func(t *testing.T) { - details := &auth.ApiKeyDetails{DisplayName: "CI", OwnedByWorkspace: "ops", ExpiresOn: tt.expiresOn} - if tt.expiresIn != 0 { - details.ExpiresAt = time.Now().UTC().Add(tt.expiresIn) - } - status := auth.CredentialStatus{ - Profile: "ci", Endpoint: xurl.MustParsef("https://meshstack.example.com"), Kind: credential.ApiKeyName, - ApiKey: &auth.ApiKeyStatus{Details: details}, - } - - shown, err := Execute(credentialOnly, status) - - require.NoError(t, err) - assert.Contains(t, shown, "\n| | CI, owned by workspace ops, "+tt.want+" |\n") - }) - }) - } -} - func TestATimeIsShownWithHowFarAwayItIs(t *testing.T) { for d, want := range map[time.Duration]string{ 30 * time.Second: "now", diff --git a/cmd/internal/testacc/api_get_test.go b/cmd/internal/testacc/api_get_test.go index f10e1de7..13c1f387 100644 --- a/cmd/internal/testacc/api_get_test.go +++ b/cmd/internal/testacc/api_get_test.go @@ -35,9 +35,6 @@ var getExceptions = map[openapi.PathTemplate]getException{ "/api/meshobjects/meshbuildingblockruns": { query: map[string]objectField{"buildingBlockUuid": {"meshBuildingBlock", "uuid"}}, }, - "/api/meshobjects/meshapikeys/self": { - apiKeyOnly: "it answers 404 to a request that authenticates with no meshApiKey", - }, "/api/meshobjects/meshbuildingblockruns/{buildingBlockRun}/plan-artifact": { skip: "the API docs say it is in development, and answers 404 in normal operation", }, @@ -48,8 +45,6 @@ type getException struct { query map[string]objectField // skip sends no GET, for the reason it gives. skip string - // apiKeyOnly sends the GET only for the API key login, for the reason it gives. - apiKeyOnly string } // objectField is a field of the metadata of the first object of a kind. @@ -57,7 +52,7 @@ type objectField struct { kind, field string } -func everyGetOperationAnswers(c *cli, apiKey bool) func(*testing.T) { +func everyGetOperationAnswers(c *cli) func(*testing.T) { return func(t *testing.T) { spec := devApiDocs(t) objects := firstObjects{spec: spec, meshStack: c.client(t)} @@ -71,9 +66,6 @@ func everyGetOperationAnswers(c *cli, apiKey bool) func(*testing.T) { if exception.skip != "" { t.Skip(exception.skip) } - if exception.apiKeyOnly != "" && !apiKey { - t.Skip(exception.apiKeyOnly) - } r, skip, err := objects.fill(t.Context(), operation, exception) require.NoError(t, err) if skip != "" { diff --git a/cmd/internal/testacc/login_test.go b/cmd/internal/testacc/login_test.go index 676d3258..71bcd35d 100644 --- a/cmd/internal/testacc/login_test.go +++ b/cmd/internal/testacc/login_test.go @@ -151,7 +151,7 @@ func TestAccBrowserLogin(t *testing.T) { completeKeycloakLogin(t, run.awaitStartURL(t), "read", admin.Username, admin.Password) require.NoErrorf(t, run.wait(), "the browser login did not finish:\n%s", run.output.String()) }) - t.Run("every GET operation answers the organization admin", everyGetOperationAnswers(c, false)) + t.Run("every GET operation answers the organization admin", everyGetOperationAnswers(c)) t.Run("--apitoken logs in with the token auth token prints", apiTokenLogsIn(c)) } @@ -238,7 +238,7 @@ func TestAccApiKeyLogin(t *testing.T) { c.setEnv(envWorkspace, workspace) } t.Run("every list command answers", everyListCommandAnswers(c)) - t.Run("every GET operation answers the API key", everyGetOperationAnswers(c, true)) + t.Run("every GET operation answers the API key", everyGetOperationAnswers(c)) t.Run("a meshObject endpoint refuses JSON, and api sends --request-json in its media type", apiAsksForJson(c)) t.Run("api picks the profile for the self link of a list answer by its URL alone, with no endpoint in the environment", apiFollowsASelfLink(c)) t.Run("a wait ends once the run has ended", waitEndsTheRun(c)) @@ -283,8 +283,6 @@ func (c *cli) authToken(t *testing.T) string { return token } -// requireAuthStatus does not check whether this meshStack lets an API key read itself through -// /self yet, so it accepts the warnings about the key's details. func requireAuthStatus(t *testing.T, c *cli, wantCredential string) { t.Helper() status, err := c.run("", "auth", "status") diff --git a/cmd/profile/model.go b/cmd/profile/model.go index cb9aaf51..7c73345d 100644 --- a/cmd/profile/model.go +++ b/cmd/profile/model.go @@ -158,7 +158,7 @@ func (m model) dialog(content string) string { return lipgloss.PlaceHorizontal(m.width, lipgloss.Center, box) } -// startForm loads the details and looks up the workspaces in the background, as both call meshStack. +// startForm loads the details and looks up the workspaces in the background, as the lookup calls meshStack. func (m model) startForm() tea.Cmd { original := m.form.original ctx, profiles := m.ctx, m.Profiles diff --git a/cmd/profile/show.go b/cmd/profile/show.go index 1697c3ed..9891cee8 100644 --- a/cmd/profile/show.go +++ b/cmd/profile/show.go @@ -5,7 +5,6 @@ import ( _ "embed" "fmt" "log/slog" - "time" "github.com/spf13/cobra" @@ -40,14 +39,8 @@ type shown struct { New bool `json:"new,omitzero"` Status *auth.CredentialStatus `json:"status,omitzero"` - // StatusError says why there is no status for a stored credential. - StatusError string `json:"statusError,omitzero"` } -// statusReadTime is shorter than a command waits for an unreachable meshStack, which is about 30s, -// as edit reads the status before it asks anything. -const statusReadTime = 5 * time.Second - // showOf has no status where p stores no usable credential, and logs a warning for it, unless p was // never logged in, which show.md.tmpl says itself. func showOf(ctx context.Context, profiles profile.Profiles, p *profile.Profile) shown { @@ -55,20 +48,12 @@ func showOf(ctx context.Context, profiles profile.Profiles, p *profile.Profile) if p.Credential == "" { return s } - ctx, cancel := context.WithTimeoutCause(ctx, statusReadTime, fmt.Errorf("meshStack did not answer within %s", statusReadTime)) - defer cancel() session, err := auth.StoredSession(ctx, p, internal.ResolveClientOptions()) if err != nil { slog.WarnContext(ctx, "Cannot read the credential of profile '"+string(p.Name)+"': "+err.Error()) return s } status := session.Status(ctx) - // Status reads what it can, and logs what it cannot, so a status read past the deadline lacks - // what meshStack would have answered. - if ctx.Err() != nil { - s.StatusError = context.Cause(ctx).Error() - return s - } s.Status = &status return s } @@ -81,8 +66,7 @@ func newShow() *cobra.Command { Long: `Show the profile another command would use, and the status of the credential it stores, which a flag or the environment would replace there. -A profile that is not stored yet is shown as a first login would create it, and nothing is stored. -Where meshStack does not answer within ` + statusReadTime.String() + `, it shows what the profile stores without the status.`, +A profile that is not stored yet is shown as a first login would create it, and nothing is stored.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { ctx := cmd.Context() diff --git a/cmd/profile/show.md.tmpl b/cmd/profile/show.md.tmpl index 69de7693..3101f905 100644 --- a/cmd/profile/show.md.tmpl +++ b/cmd/profile/show.md.tmpl @@ -6,7 +6,7 @@ | --- | --- | | Endpoint | {{with .Endpoint.URL}}{{cell .}}{{else}}none{{end}} | | Default workspace | {{or .DefaultWorkspace "none"}} | -| Credential | {{with .StatusError}}{{$.Credential.Label}}, whose status could not be read: {{cell .}}{{else}}none, run `meshstack login -p {{.Name}}`{{end}} | +| Credential | none, run `meshstack login -p {{.Name}}` | {{- end}} {{- if .New}} diff --git a/cmd/profile/show_test.go b/cmd/profile/show_test.go index bba592af..6d02cc75 100644 --- a/cmd/profile/show_test.go +++ b/cmd/profile/show_test.go @@ -1,23 +1,16 @@ package profile import ( - "context" "encoding/json/v2" "log/slog" "maps" - gohttp "net/http" - "net/http/httptest" "slices" "testing" - "time" - "uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/meshcloud/meshstack-cli/client/types/xurl" "github.com/meshcloud/meshstack-cli/cmd/internal" - "github.com/meshcloud/meshstack-cli/cmd/internal/markdown" "github.com/meshcloud/meshstack-cli/internal/auth/credential" "github.com/meshcloud/meshstack-cli/internal/logs" "github.com/meshcloud/meshstack-cli/internal/profile" @@ -138,28 +131,3 @@ func requireStoredNames(t *testing.T, want ...profile.Name) { require.NoError(t, err) assert.ElementsMatch(t, want, slices.Collect(maps.Keys(stored.Profiles))) } - -func TestShowSaysWhereTheStatusCannotBeReadInTime(t *testing.T) { - unreachable := httptest.NewServer(gohttp.HandlerFunc(func(_ gohttp.ResponseWriter, r *gohttp.Request) { - <-r.Context().Done() - })) - t.Cleanup(unreachable.CloseClientConnections) - endpoint := xurl.MustParsef("%s", unreachable.URL) - profiles := storedProfiles(t, profile.Profile{Name: "ci", Endpoint: endpoint, Credential: credential.ApiKeyName}) - credentials, err := profiles.Profiles["ci"].Credentials(t.Context()) - require.NoError(t, err) - credentials.Set(&credential.ApiKey{Endpoint: endpoint, ClientId: uuid.MustParse("11111111-45bf-42ba-a965-2097b9d0d181"), ClientSecret: "test-secret"}) - require.NoError(t, credentials.Store(t.Context())) - logs.Capture(t) - // A deadline of the caller stands in for statusReadTime, which a test would wait for in full. - ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) - defer cancel() - - s := showOf(ctx, profiles, profiles.Profiles["ci"]) - - assert.Nil(t, s.Status) - shownMarkdown, err := markdown.Execute(showTemplate, s) - require.NoError(t, err) - assert.Contains(t, shownMarkdown, "| Endpoint | "+unreachable.URL+" |\n") - assert.Contains(t, shownMarkdown, "| Credential | API key, whose status could not be read: context deadline exceeded |\n") -} diff --git a/docs/profile.md b/docs/profile.md index 58fc6c03..08f1e1ce 100644 --- a/docs/profile.md +++ b/docs/profile.md @@ -54,9 +54,8 @@ Without a terminal, `add` and `edit` ask one question per line on standard input `--profile`, `show` shows the profile another command would use, and `edit` and `delete` ask which profile to change: from a list on a terminal, and by its number otherwise. -`show`, and `edit` before it asks, read the status of the stored credential, for which meshStack -answers about an API key. Where it does not answer within 5s, they show what the profile stores, -and say that the status could not be read. +`show`, and `edit` before it asks, read the status of the stored credential from the files of the +profile. They do not call meshStack for it. Where standard input ends before an answer, the question takes its default answer, so the flags can give every answer: diff --git a/internal/auth/status.go b/internal/auth/status.go index 539a3568..8278183d 100644 --- a/internal/auth/status.go +++ b/internal/auth/status.go @@ -11,10 +11,8 @@ import ( "time" "uuid" - "github.com/meshcloud/meshstack-cli/client" "github.com/meshcloud/meshstack-cli/client/types/xurl" "github.com/meshcloud/meshstack-cli/internal/auth/credential" - "github.com/meshcloud/meshstack-cli/internal/http" "github.com/meshcloud/meshstack-cli/internal/meshstack" "github.com/meshcloud/meshstack-cli/internal/oidc/jwt" "github.com/meshcloud/meshstack-cli/internal/profile" @@ -33,8 +31,7 @@ type CredentialStatus struct { // cached. Token *TokenStatus `json:"token,omitzero"` - // OidcLogin is set for a browser login. ApiKey is set for an API key, and for an API token that - // an API key minted. + // OidcLogin is set for a browser login. ApiKey is set for an API key. OidcLogin *OidcLoginStatus `json:"oidcLogin,omitzero"` ApiKey *ApiKeyStatus `json:"apiKey,omitzero"` @@ -94,26 +91,11 @@ func BrowserLoginEnded(ctx context.Context, p *profile.Profile) bool { } type ApiKeyStatus struct { - ClientId uuid.UUID `json:"clientId"` - Details *ApiKeyDetails `json:"details,omitzero"` + ClientId uuid.UUID `json:"clientId"` } -type ApiKeyDetails struct { - DisplayName string `json:"displayName"` - OwnedByWorkspace string `json:"ownedByWorkspace"` - ExpiresAt time.Time `json:"expiresAt,omitzero"` - // ExpiresOn is the expiry date, set only by a meshStack that does not send the expiry time - // yet. It is not turned into a time: the date of an ephemeral key is the day it expires on, - // not the last day it works. - ExpiresOn string `json:"expiresOn,omitzero"` - Permissions []string `json:"permissions"` - PermissionGroups client.ApiKeyPermissions `json:"-"` -} - -const statusTimeout = 10 * time.Second - -// Status never renews a browser login, which would rotate its refresh token. Only an API key, or an -// API token it minted, calls meshStack, to read the key's own details. +// Status reads only the stored credential and its cached token, and calls no meshStack. It never +// renews a browser login, which would rotate its refresh token. func (s Session) Status(ctx context.Context) CredentialStatus { status := CredentialStatus{ Profile: s.CurrentProfile.Name, @@ -137,18 +119,10 @@ func (s Session) Status(ctx context.Context) CredentialStatus { token, found = oidcTokenFor(cred, workspace) } case *credential.ApiKey: - status.ApiKey = &ApiKeyStatus{ClientId: cred.ClientId, Details: apiKeyDetails(ctx, s.Client)} + status.ApiKey = &ApiKeyStatus{ClientId: cred.ClientId} token, found = cred.CachedToken(ctx, s.getWorkspace) case *credential.Manual: token, found = cred.CachedToken(ctx, s.getWorkspace) - // A pasted API token is most likely the token of a building block run, which its ephemeral - // API key minted. An expired one would only fail to read the key. - if clientId, err := uuid.Parse(token.GetClaim(jwt.ClientIdClaim)); err == nil { - status.ApiKey = &ApiKeyStatus{ClientId: clientId} - if !tokenStatusOf(token).Expired() { - status.ApiKey.Details = apiKeyDetails(ctx, s.Client) - } - } } if found { status.Token = tokenStatusOf(token) @@ -218,46 +192,3 @@ func tokenStatusOf(token jwt.JWT) *TokenStatus { AccessLevel: level, } } - -func apiKeyDetails(ctx context.Context, newClient func() (client.Client, error)) *ApiKeyDetails { - c, err := newClient() - if err != nil { - slog.WarnContext(ctx, "Cannot read the details of the API key: "+err.Error()) - return nil - } - readCtx, cancel := context.WithTimeoutCause(ctx, statusTimeout, - fmt.Errorf("meshStack did not answer within %s", statusTimeout)) - defer cancel() - apiKey, err := c.ApiKey.ReadSelf(readCtx) - if cause := context.Cause(readCtx); err != nil && cause != nil { - err = cause - } - switch httpErr, isHttpErr := errors.AsType[http.Error](err); { - // A meshStack without /self takes it for the uuid of a key: without APIKEY_LIST that is a 403, - // and with it a 404, which ReadSelf returns as nil. - case isHttpErr && httpErr.IsForbidden(), err == nil && apiKey == nil: - slog.WarnContext(ctx, "Cannot read the details of the API key: this meshStack does not let an API key read itself yet") - return nil - case err != nil: - slog.WarnContext(ctx, "Cannot read the details of the API key: "+err.Error()) - return nil - } - codes := make([]string, 0, len(apiKey.Spec.Permissions)) - for _, code := range apiKey.Spec.Permissions { - codes = append(codes, string(code)) - } - slices.Sort(codes) - details := &ApiKeyDetails{ - DisplayName: apiKey.Spec.DisplayName, - OwnedByWorkspace: apiKey.Metadata.OwnedByWorkspace, - Permissions: codes, - PermissionGroups: client.Permissions.Only(apiKey.Spec.Permissions), - } - if apiKey.Status != nil { - details.ExpiresAt = apiKey.Status.ExpiresAt - } - if details.ExpiresAt.IsZero() && apiKey.Spec.ExpiresAt != nil { - details.ExpiresOn = *apiKey.Spec.ExpiresAt - } - return details -} diff --git a/internal/auth/status_test.go b/internal/auth/status_test.go index a0436461..e30f3cb5 100644 --- a/internal/auth/status_test.go +++ b/internal/auth/status_test.go @@ -3,9 +3,7 @@ package auth_test import ( "encoding/base64" "encoding/json/v2" - "fmt" "log/slog" - gohttp "net/http" "testing" "time" "uuid" @@ -13,7 +11,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/meshcloud/meshstack-cli/client" "github.com/meshcloud/meshstack-cli/client/types/xurl" "github.com/meshcloud/meshstack-cli/internal/auth" "github.com/meshcloud/meshstack-cli/internal/auth/credential" @@ -78,77 +75,24 @@ func TestStatusOfABrowserLogin(t *testing.T) { }) } -func TestStatusOfAnApiKeyReadsTheKeyFromMeshStack(t *testing.T) { - tests := []struct { - name string - status int - body string - want *auth.ApiKeyDetails - wantWarn string - }{ - { - name: "a key reads itself", - status: gohttp.StatusOK, - body: `{"metadata":{"uuid":"` + testApiKey1.ClientId + `","ownedByWorkspace":"ops"},` + - `"spec":{"displayName":"CI","permissions":["WORKSPACE_LIST","APIKEY_LIST"],"expiresAt":"2027-01-31"},` + - `"status":{"clientId":"` + testApiKey1.ClientId + `","expiresAt":"2027-02-01T00:00:00Z"}}`, - want: &auth.ApiKeyDetails{ - DisplayName: "CI", OwnedByWorkspace: "ops", ExpiresAt: time.Date(2027, 2, 1, 0, 0, 0, 0, time.UTC), - Permissions: []string{"APIKEY_LIST", "WORKSPACE_LIST"}, - PermissionGroups: client.ApiKeyPermissions{ - {Name: "API Keys", Actions: [][]client.ApiPermission{{"APIKEY_LIST"}}}, - {Name: "Workspaces", Actions: [][]client.ApiPermission{{"WORKSPACE_LIST"}}}, - }, - }, - }, - { - name: "a meshStack that sends no expiry time leaves the expiry date", - status: gohttp.StatusOK, - body: `{"metadata":{"uuid":"` + testApiKey1.ClientId + `","ownedByWorkspace":"ops"},` + - `"spec":{"displayName":"CI","permissions":[],"expiresAt":"2027-01-31"},"status":{"clientId":"` + testApiKey1.ClientId + `"}}`, - want: &auth.ApiKeyDetails{DisplayName: "CI", OwnedByWorkspace: "ops", ExpiresOn: "2027-01-31", Permissions: []string{}}, - }, - {name: "a meshStack without /self forbids it to a key without APIKEY_LIST", status: gohttp.StatusForbidden, wantWarn: "does not let an API key read itself"}, - {name: "a meshStack without /self finds no key of that uuid", status: gohttp.StatusNotFound, wantWarn: "does not let an API key read itself"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - server := newTestServer(t) - // The version check would ask GitHub for the latest release. - t.Setenv(meshstack.SkipVersionCheckSetting.EnvKey(), "true") - server.Route("/api/meshobjects/meshapikeys/self", func(resp gohttp.ResponseWriter, _ *gohttp.Request) { - resp.WriteHeader(tt.status) - _, _ = fmt.Fprint(resp, tt.body) - }) - setApiKeyEnv(t, testApiKey1) - captured := logs.Capture(t) - - session, err := auth.ResolveSession(t.Context(), testSessionOpts) - require.NoError(t, err) - status := session.Status(t.Context()) - - assert.Equal(t, []string{"env MESHSTACK_API_KEY", "env MESHSTACK_API_SECRET"}, status.Sources) - require.NotNil(t, status.ApiKey) - assert.Equal(t, testApiKey1.ClientId, status.ApiKey.ClientId.String()) - assert.Equal(t, tt.want, status.ApiKey.Details) - if tt.wantWarn != "" { - assert.Contains(t, captured.String(), "level=WARN") - assert.Contains(t, captured.String(), tt.wantWarn) - } else { - assert.Empty(t, captured.Records(slog.LevelWarn)) - } - assert.NotNil(t, status.Token, "reading the key minted a token") - }) - } +func TestStatusOfAnApiKeyIsReadFromFilesAndTheEnvironment(t *testing.T) { + newTestServer(t) + setApiKeyEnv(t, testApiKey1) + captured := logs.Capture(t) + + session, err := auth.ResolveSession(t.Context(), testSessionOpts) + require.NoError(t, err) + status := session.Status(t.Context()) + + assert.Equal(t, []string{"env MESHSTACK_API_KEY", "env MESHSTACK_API_SECRET"}, status.Sources) + require.NotNil(t, status.ApiKey) + assert.Equal(t, testApiKey1.ClientId, status.ApiKey.ClientId.String()) + assert.Nil(t, status.Token, "no token is cached before the key is first used") + assert.Empty(t, captured.Records(slog.LevelWarn)) } func TestStatusOfAnApiToken(t *testing.T) { - server := newTestServer(t) - t.Setenv(meshstack.SkipVersionCheckSetting.EnvKey(), "true") - server.Route("/api/meshobjects/meshapikeys/self", func(resp gohttp.ResponseWriter, _ *gohttp.Request) { - _, _ = fmt.Fprint(resp, `{"metadata":{"uuid":"`+testApiKey1.ClientId+`","ownedByWorkspace":"ops"},`+ - `"spec":{"displayName":"run 42","permissions":["BUILDINGBLOCK_SAVE"]}}`) - }) + newTestServer(t) statusWithTokenInTheEnvironment := func(t *testing.T, claims map[string]any) auth.CredentialStatus { t.Helper() t.Setenv(auth.ApiTokenSetting.EnvKey(), testToken(t, claims).String()) @@ -172,26 +116,21 @@ func TestStatusOfAnApiToken(t *testing.T) { assert.Equal(t, "stored", status.Unused[0].Token.User) }) - t.Run("the token of a building block run reads its ephemeral API key", func(t *testing.T) { + t.Run("the token of a building block run shows the client id of its API key", func(t *testing.T) { status := statusWithTokenInTheEnvironment(t, map[string]any{"exp": validFor(time.Hour), "client_id": testApiKey1.ClientId}) assert.Equal(t, credential.ManualName, status.Kind) - require.NotNil(t, status.ApiKey) - assert.Equal(t, testApiKey1.ClientId, status.ApiKey.ClientId.String()) - require.NotNil(t, status.ApiKey.Details) - assert.Equal(t, "run 42", status.ApiKey.Details.DisplayName) - assert.Equal(t, []string{"BUILDINGBLOCK_SAVE"}, status.ApiKey.Details.Permissions) + require.NotNil(t, status.Token) + assert.Equal(t, testApiKey1.ClientId, status.Token.ClientId) }) - t.Run("an expired token says so, and cannot read its API key", func(t *testing.T) { + t.Run("an expired token says so", func(t *testing.T) { captured := logs.Capture(t) status := statusWithTokenInTheEnvironment(t, map[string]any{"exp": validFor(-time.Hour), "client_id": testApiKey1.ClientId}) require.NotNil(t, status.Token) assert.True(t, status.Token.Expired()) assert.Equal(t, testApiKey1.ClientId, status.Token.ClientId) - require.NotNil(t, status.ApiKey) - assert.Nil(t, status.ApiKey.Details) warnings := captured.Lines(slog.LevelWarn) require.Len(t, warnings, 1) assert.Contains(t, warnings[0], "set a credential over 'manual' stored in profile 'default'") From d6de970d6c12643db253eee1c20a48058418a72a Mon Sep 17 00:00:00 2001 From: Andreas Grub Date: Mon, 2 Nov 2026 14:26:15 +0100 Subject: [PATCH 2/5] feat: show meshStack errors by their fields, with the client version hint The CLI reads the error body of meshStack strictly. A known body shows its message, errorCode and errorId, and its clientVersionHint on a line of its own. A body in any other format shows the status and the raw body. Co-Authored-By: Claude Opus 5.5 --- cmd/api/api.go | 19 ++++--- cmd/api/api_test.go | 15 ++++-- cmd/buildingblock/tfstate/tfstate_test.go | 4 +- internal/http/http_error.go | 65 ++++++++++++++++++++++- internal/http/http_error_test.go | 41 ++++++++++++++ 5 files changed, 131 insertions(+), 13 deletions(-) create mode 100644 internal/http/http_error_test.go diff --git a/cmd/api/api.go b/cmd/api/api.go index b9424618..f7c6f082 100644 --- a/cmd/api/api.go +++ b/cmd/api/api.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "encoding/json/jsontext" - "encoding/json/v2" "errors" "fmt" "io" @@ -96,11 +95,11 @@ Replace the same command line with 'api-docs' in place of 'api' to describe the } // The body is on stdout already, so the error says only what it adds. if httpErr.IsClientError() && !httpErr.IsUnauthorized() && !httpErr.IsForbidden() && !r.objectNotFound(httpErr) { - return fmt.Errorf("meshStack answered HTTP %d. Run %s to see what the API takes", - httpErr.StatusCode, docsCommand(cmd, args)) + return withHint(fmt.Sprintf("%s. Run %s to see what the API takes", + httpErr.Summary(), docsCommand(cmd, args)), httpErr) } // What wraps the HTTP error, such as the auth scope of a 403, stays. - return errors.New(strings.Replace(err.Error(), httpErr.Error(), fmt.Sprintf("meshStack answered HTTP %d", httpErr.StatusCode), 1)) + return withHint(strings.Replace(err.Error(), httpErr.Error(), httpErr.Summary(), 1), httpErr) } if err != nil { return err @@ -120,10 +119,16 @@ func (r *request) objectNotFound(httpErr client.HttpError) bool { if !httpErr.IsNotFound() || !r.documented { return false } - var answer struct { - ErrorCode string `json:"errorCode"` + return httpErr.ErrorCode() != noHandlerFound +} + +// withHint adds the hint on a line of its own. Stdout carries the raw body, which the error does not +// repeat. +func withHint(message string, httpErr client.HttpError) error { + if hint := httpErr.ClientVersionHint(); hint != "" { + message += "\n" + hint } - return json.Unmarshal(httpErr.ResponseBody, &answer) != nil || answer.ErrorCode != noHandlerFound + return errors.New(message) } // noHandlerFound is the errorCode of meshStack's answer to a path it serves nothing at. diff --git a/cmd/api/api_test.go b/cmd/api/api_test.go index abe6387c..68dfdf69 100644 --- a/cmd/api/api_test.go +++ b/cmd/api/api_test.go @@ -128,9 +128,9 @@ func TestApi(t *testing.T) { t.Run("writes the body of a failed answer, and fails with a hint to api-docs on a client error", func(t *testing.T) { for status, wantError := range map[int]string{ - gohttp.StatusConflict: "meshStack answered HTTP 409. Run meshstack api-docs /api/x -H 'Accept: application/json' -X DELETE to see what the API takes", - gohttp.StatusBadGateway: "meshStack answered HTTP 502", - gohttp.StatusForbidden: "auth scope profile default: meshStack answered HTTP 403", + gohttp.StatusConflict: "meshStack answered HTTP 409: still in use. Run meshstack api-docs /api/x -H 'Accept: application/json' -X DELETE to see what the API takes", + gohttp.StatusBadGateway: "meshStack answered HTTP 502: still in use", + gohttp.StatusForbidden: "auth scope profile default: meshStack answered HTTP 403: still in use", } { meshStack.answer(status, `{"message":"still in use"}`) @@ -141,6 +141,15 @@ func TestApi(t *testing.T) { } }) + t.Run("adds the clientVersionHint of a failed answer to the error", func(t *testing.T) { + meshStack.answer(gohttp.StatusNotFound, `{"errorCode":"NoHandlerFound","message":"no handler","errorId":"abc","clientVersionHint":"Upgrade the meshStack CLI to v0.9.0."}`) + + stdout, err := meshStack.run(t, api.New(), "", "/api/x", "-H", "Accept: application/json") + + require.EqualError(t, err, "meshStack answered HTTP 404: no handler (errorCode NoHandlerFound, errorId abc). Run meshstack api-docs /api/x -X GET -H 'Accept: application/json' to see what the API takes\nUpgrade the meshStack CLI to v0.9.0.") + assert.Contains(t, stdout, "clientVersionHint") + }) + t.Run("asks for the media type the API docs list for a path of no version", func(t *testing.T) { meshStack.answer(gohttp.StatusOK, "plan") diff --git a/cmd/buildingblock/tfstate/tfstate_test.go b/cmd/buildingblock/tfstate/tfstate_test.go index fe6f16d3..c11db7de 100644 --- a/cmd/buildingblock/tfstate/tfstate_test.go +++ b/cmd/buildingblock/tfstate/tfstate_test.go @@ -254,7 +254,7 @@ func TestTfstate(t *testing.T) { require.NoError(t, ran.err) assert.Contains(t, ran.log, "level=WARN") - assert.Contains(t, ran.log, "http error 500") + assert.Contains(t, ran.log, "meshStack answered HTTP 500") }) t.Run("a refusal of meshStack says which rights the state takes", func(t *testing.T) { @@ -265,7 +265,7 @@ func TestTfstate(t *testing.T) { err := execChild(t, "GET").err require.Error(t, err) - assert.Contains(t, err.Error(), "http error 403") + assert.Contains(t, err.Error(), "meshStack answered HTTP 403") assert.Contains(t, err.Error(), "MANAGED_TFSTATE_LIST") assert.Contains(t, err.Error(), "meshstack login --apikey") }) diff --git a/internal/http/http_error.go b/internal/http/http_error.go index 57914475..680abe13 100644 --- a/internal/http/http_error.go +++ b/internal/http/http_error.go @@ -1,8 +1,10 @@ package http import ( + "encoding/json/v2" "fmt" gohttp "net/http" + "strings" ) type Error struct { @@ -10,8 +12,61 @@ type Error struct { ResponseBody []byte } +// Error describes the response from its fields, with the clientVersionHint on a line of its own. A +// response of another format is shown as it came. func (e Error) Error() string { - return fmt.Sprintf("http error %d, response '%s'", e.StatusCode, string(e.ResponseBody)) + body, ok := e.body() + if !ok { + return fmt.Sprintf("%s, but the response is not in the expected error format: '%s'", e.Summary(), string(e.ResponseBody)) + } + if body.ClientVersionHint != "" { + return e.Summary() + "\n" + body.ClientVersionHint + } + return e.Summary() +} + +// Summary is Error without the clientVersionHint and without a body of an unexpected format. +func (e Error) Summary() string { + summary := fmt.Sprintf("meshStack answered HTTP %d", e.StatusCode) + body, ok := e.body() + if !ok { + return summary + } + if body.Message != "" { + summary += ": " + body.Message + } + var details []string + if body.ErrorCode != "" { + details = append(details, "errorCode "+body.ErrorCode) + } + if body.ErrorId != "" { + details = append(details, "errorId "+body.ErrorId) + } + if len(details) > 0 { + summary += " (" + strings.Join(details, ", ") + ")" + } + return summary +} + +// errorBody is the body of every error response of meshStack, the 401 of a request without +// credentials included. +type errorBody struct { + ErrorCode string `json:"errorCode"` + Message string `json:"message"` + ErrorId string `json:"errorId"` + ClientVersionHint string `json:"clientVersionHint"` +} + +func (e Error) ErrorCode() string { + body, _ := e.body() + return body.ErrorCode +} + +// ClientVersionHint is what meshStack tells a client that is too old for it, and is empty for any other +// response. +func (e Error) ClientVersionHint() string { + body, _ := e.body() + return body.ClientVersionHint } func (e Error) IsClientError() bool { @@ -37,3 +92,11 @@ func (e Error) IsConflict() bool { func (e Error) IsLocked() bool { return e.StatusCode == gohttp.StatusLocked } + +// body is not ok for a response that is no JSON object of exactly the fields of errorBody. +func (e Error) body() (body errorBody, ok bool) { + if err := json.Unmarshal(e.ResponseBody, &body, json.RejectUnknownMembers(true)); err != nil { + return errorBody{}, false + } + return body, body.ErrorCode != "" || body.Message != "" +} diff --git a/internal/http/http_error_test.go b/internal/http/http_error_test.go new file mode 100644 index 00000000..6ee3109b --- /dev/null +++ b/internal/http/http_error_test.go @@ -0,0 +1,41 @@ +package http + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrorIsReadFromTheFieldsOfTheBody(t *testing.T) { + t.Run("shows the clientVersionHint on a line of its own", func(t *testing.T) { + err := Error{StatusCode: 404, ResponseBody: []byte( + `{"errorCode":"NoHandlerFound","message":"not found","errorId":"abc","clientVersionHint":"Upgrade to v0.9.0."}`)} + + assert.Equal(t, "Upgrade to v0.9.0.", err.ClientVersionHint()) + assert.Equal(t, "NoHandlerFound", err.ErrorCode()) + assert.Equal(t, "meshStack answered HTTP 404: not found (errorCode NoHandlerFound, errorId abc)\nUpgrade to v0.9.0.", err.Error()) + }) + + t.Run("shows no hint where there is none", func(t *testing.T) { + err := Error{StatusCode: 422, ResponseBody: []byte( + `{"errorCode":"Invalid","message":"bad","errorId":"x1"}`)} + + assert.Empty(t, err.ClientVersionHint()) + assert.Equal(t, "meshStack answered HTTP 422: bad (errorCode Invalid, errorId x1)", err.Error()) + }) + + for name, body := range map[string]string{ + "a field it does not know": `{"errorCode":"X","message":"m","errorId":"i","other":1}`, + "no JSON": `Bad Gateway`, + "no body": ``, + "an empty object": `{}`, + } { + t.Run("shows the raw body for "+name, func(t *testing.T) { + err := Error{StatusCode: 502, ResponseBody: []byte(body)} + + assert.Empty(t, err.ClientVersionHint()) + assert.Equal(t, "meshStack answered HTTP 502, but the response is not in the expected error format: '"+body+"'", err.Error()) + assert.Equal(t, "meshStack answered HTTP 502", err.Summary()) + }) + } +} From c5b0de82a9b9e39b4de1d1efc3852d1c6618c0cd Mon Sep 17 00:00:00 2001 From: Andreas Grub Date: Mon, 2 Nov 2026 20:37:30 +0100 Subject: [PATCH 3/5] test: expect the fields of the meshStack error in the 406 message Co-Authored-By: Claude Opus 5.5 --- cmd/internal/testacc/api_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/internal/testacc/api_test.go b/cmd/internal/testacc/api_test.go index f58beb68..7e285340 100644 --- a/cmd/internal/testacc/api_test.go +++ b/cmd/internal/testacc/api_test.go @@ -15,8 +15,9 @@ import ( func apiAsksForJson(c *cli) func(*testing.T) { return func(t *testing.T) { output, err := c.run("", "api", "/api/meshobjects/meshworkspaces", "-H", "Accept: application/json") - require.EqualErrorf(t, err, "meshStack answered HTTP 406. Run meshstack api-docs /api/meshobjects/meshworkspaces "+ - "-X GET -H 'Accept: application/json' to see what the API takes", "a meshObject endpoint refuses application/json, output:\n%s", output) + require.ErrorContainsf(t, err, "meshStack answered HTTP 406: value in 'Accept' header not supported. (errorCode HttpMediaTypeNotAcceptable, errorId ", + "a meshObject endpoint refuses application/json, output:\n%s", output) + require.ErrorContains(t, err, "). Run meshstack api-docs /api/meshobjects/meshworkspaces -X GET -H 'Accept: application/json' to see what the API takes") // A body of no metadata, which meshStack refuses before it creates anything. output, err = c.run(`{}`, "api", "-X", "POST", "/api/meshobjects/meshworkspaces", "--request-json", "-") From e7f8f9b42a8a0507a834af75c6459b69c9802d62 Mon Sep 17 00:00:00 2001 From: Andreas Grub Date: Tue, 3 Nov 2026 13:26:14 +0100 Subject: [PATCH 4/5] fix: read a meshStack error that has fields the CLI does not know Co-Authored-By: Claude Opus 5.5 --- internal/http/http_error.go | 3 +-- internal/http/http_error_test.go | 13 +++++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/http/http_error.go b/internal/http/http_error.go index 680abe13..e20c3a89 100644 --- a/internal/http/http_error.go +++ b/internal/http/http_error.go @@ -93,9 +93,8 @@ func (e Error) IsLocked() bool { return e.StatusCode == gohttp.StatusLocked } -// body is not ok for a response that is no JSON object of exactly the fields of errorBody. func (e Error) body() (body errorBody, ok bool) { - if err := json.Unmarshal(e.ResponseBody, &body, json.RejectUnknownMembers(true)); err != nil { + if err := json.Unmarshal(e.ResponseBody, &body); err != nil { return errorBody{}, false } return body, body.ErrorCode != "" || body.Message != "" diff --git a/internal/http/http_error_test.go b/internal/http/http_error_test.go index 6ee3109b..e868ccca 100644 --- a/internal/http/http_error_test.go +++ b/internal/http/http_error_test.go @@ -16,6 +16,12 @@ func TestErrorIsReadFromTheFieldsOfTheBody(t *testing.T) { assert.Equal(t, "meshStack answered HTTP 404: not found (errorCode NoHandlerFound, errorId abc)\nUpgrade to v0.9.0.", err.Error()) }) + t.Run("ignores a field it does not know", func(t *testing.T) { + err := Error{StatusCode: 409, ResponseBody: []byte(`{"errorCode":"X","message":"m","other":1}`)} + + assert.Equal(t, "meshStack answered HTTP 409: m (errorCode X)", err.Error()) + }) + t.Run("shows no hint where there is none", func(t *testing.T) { err := Error{StatusCode: 422, ResponseBody: []byte( `{"errorCode":"Invalid","message":"bad","errorId":"x1"}`)} @@ -25,10 +31,9 @@ func TestErrorIsReadFromTheFieldsOfTheBody(t *testing.T) { }) for name, body := range map[string]string{ - "a field it does not know": `{"errorCode":"X","message":"m","errorId":"i","other":1}`, - "no JSON": `Bad Gateway`, - "no body": ``, - "an empty object": `{}`, + "no JSON": `Bad Gateway`, + "no body": ``, + "an empty object": `{}`, } { t.Run("shows the raw body for "+name, func(t *testing.T) { err := Error{StatusCode: 502, ResponseBody: []byte(body)} From 85863f254cbfa239ad39ad5221284ac29afcc388 Mon Sep 17 00:00:00 2001 From: Andreas Grub Date: Tue, 3 Nov 2026 13:26:14 +0100 Subject: [PATCH 5/5] fix(api): point to api-docs on a 404 only for a path the docs do not list Co-Authored-By: Claude Opus 5.5 --- cmd/api/api.go | 18 +++++++++++------- cmd/api/api_test.go | 15 ++++++++++++++- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/cmd/api/api.go b/cmd/api/api.go index f7c6f082..02f6c4bc 100644 --- a/cmd/api/api.go +++ b/cmd/api/api.go @@ -94,7 +94,7 @@ Replace the same command line with 'api-docs' in place of 'api' to describe the return writeErr } // The body is on stdout already, so the error says only what it adds. - if httpErr.IsClientError() && !httpErr.IsUnauthorized() && !httpErr.IsForbidden() && !r.objectNotFound(httpErr) { + if r.docsHelpWith(httpErr) { return withHint(fmt.Sprintf("%s. Run %s to see what the API takes", httpErr.Summary(), docsCommand(cmd, args)), httpErr) } @@ -113,13 +113,17 @@ Replace the same command line with 'api-docs' in place of 'api' to describe the return cmd } -// objectNotFound tells a 404 for a path that exists, whose object is missing, from one for a path that -// does not: only the latter is something the API docs help with. -func (r *request) objectNotFound(httpErr client.HttpError) bool { - if !httpErr.IsNotFound() || !r.documented { +// docsHelpWith is false for a 404 of a path meshStack serves nothing at, and of an object missing at a +// documented path: the API docs say nothing about either. +func (r *request) docsHelpWith(httpErr client.HttpError) bool { + switch { + case !httpErr.IsClientError(), httpErr.IsUnauthorized(), httpErr.IsForbidden(): return false + case httpErr.IsNotFound(): + return !r.documented && httpErr.ErrorCode() != noHandlerFound + default: + return true } - return httpErr.ErrorCode() != noHandlerFound } // withHint adds the hint on a line of its own. Stdout carries the raw body, which the error does not @@ -154,7 +158,7 @@ func (r *request) negotiate(ctx context.Context, spec openapi.Spec, selector ope // The request goes out as given where the docs know no better, because they can lag behind the // meshStack it is sent to. selected, err := spec.Select(selector) - r.documented = err == nil + r.documented = err == nil && len(selected.Operations) > 0 if err != nil { if !openapi.ParseMediaType(r.header.Get("Accept")).ApiVersion.IsZero() { slog.WarnContext(ctx, fmt.Sprintf("Sending the request as given: %s", err)) diff --git a/cmd/api/api_test.go b/cmd/api/api_test.go index 68dfdf69..905ce92c 100644 --- a/cmd/api/api_test.go +++ b/cmd/api/api_test.go @@ -141,12 +141,25 @@ func TestApi(t *testing.T) { } }) + t.Run("points to api-docs on a 404 only for a path the API docs do not list", func(t *testing.T) { + for path, wantError := range map[string]string{ + "/api/x": "meshStack answered HTTP 404: gone (errorCode NotFound). Run meshstack api-docs /api/x -X GET -H 'Accept: application/json' to see what the API takes", + "/api/meshobjects/meshtenants/0c9ef6d1-5c5d-4b0c-9a4b-1e0e3c1f2a3b": "meshStack answered HTTP 404: gone (errorCode NotFound)", + } { + meshStack.answer(gohttp.StatusNotFound, `{"errorCode":"NotFound","message":"gone"}`) + + _, err := meshStack.run(t, api.New(), "", path, "-H", "Accept: application/json") + + require.EqualError(t, err, wantError) + } + }) + t.Run("adds the clientVersionHint of a failed answer to the error", func(t *testing.T) { meshStack.answer(gohttp.StatusNotFound, `{"errorCode":"NoHandlerFound","message":"no handler","errorId":"abc","clientVersionHint":"Upgrade the meshStack CLI to v0.9.0."}`) stdout, err := meshStack.run(t, api.New(), "", "/api/x", "-H", "Accept: application/json") - require.EqualError(t, err, "meshStack answered HTTP 404: no handler (errorCode NoHandlerFound, errorId abc). Run meshstack api-docs /api/x -X GET -H 'Accept: application/json' to see what the API takes\nUpgrade the meshStack CLI to v0.9.0.") + require.EqualError(t, err, "meshStack answered HTTP 404: no handler (errorCode NoHandlerFound, errorId abc)\nUpgrade the meshStack CLI to v0.9.0.") assert.Contains(t, stdout, "clientVersionHint") })