diff --git a/README.md b/README.md index 3755395..051e3aa 100644 --- a/README.md +++ b/README.md @@ -88,5 +88,5 @@ flagsmith flag list # list the flags in the current environment ## Conventions - `--json` (or `FLAGSMITH_JSON_OUTPUT`) for machine-readable output; `--jq ` to filter it. -- Static credentials: `FLAGSMITH_API_KEY` (Admin API), `FLAGSMITH_ENVIRONMENT_KEY` (SDK). +- Static credentials: `FLAGSMITH_API_KEY` (Admin API), `FLAGSMITH_ENVIRONMENT_KEY` (SDK). When self-hosting Flagsmith, append the host and port, doubling `-` and writing `.` and `:` as `_`: `https://flagsmith-staging.com:8000` reads `FLAGSMITH_API_KEY_flagsmith__staging_com_8000`. - Self-hosted: `--api-url` or `FLAGSMITH_API_URL`. diff --git a/go.mod b/go.mod index a7615ad..d3229a8 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.26 require ( github.com/Flagsmith/flagsmith-go-client/v5 v5.1.0 + github.com/blang/semver/v4 v4.0.0 github.com/charmbracelet/huh v1.0.0 github.com/fatih/color v1.19.0 github.com/itchyny/gojq v0.12.19 @@ -18,7 +19,6 @@ require ( require ( github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect - github.com/blang/semver/v4 v4.0.0 // indirect github.com/catppuccin/go v0.3.0 // indirect github.com/charmbracelet/bubbles v0.21.1-0.20250623103423-23b8fd6302d7 // indirect github.com/charmbracelet/bubbletea v1.3.6 // indirect diff --git a/internal/api/version.go b/internal/api/version.go new file mode 100644 index 0000000..bb96870 --- /dev/null +++ b/internal/api/version.go @@ -0,0 +1,42 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "strings" +) + +// ServerVersion reports the version of the Flagsmith serving apiURL, from the +// unauthenticated /version endpoint — it answers what an instance supports +// before there is any credential to ask with. The value is the deployed image +// tag, which self-hosted instances often set to something that is not a version +// at all ("latest", a commit sha), so callers must read an unparseable result as +// unknown rather than old. +func ServerVersion(ctx context.Context, httpClient *http.Client, apiURL string) (string, error) { + u := strings.TrimRight(apiURL, "/") + "/version/" + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil) + if err != nil { + return "", err + } + resp, err := httpClient.Do(req) + if err != nil { + return "", fmt.Errorf("reaching %s: %w", u, err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("%s returned %s", u, resp.Status) + } + var doc struct { + ImageTag string `json:"image_tag"` + } + if err := json.NewDecoder(resp.Body).Decode(&doc); err != nil { + return "", fmt.Errorf("decoding %s: %w", u, err) + } + if doc.ImageTag == "" { + return "", errors.New(u + " carries no image tag") + } + return doc.ImageTag, nil +} diff --git a/internal/api/version_test.go b/internal/api/version_test.go new file mode 100644 index 0000000..fda678b --- /dev/null +++ b/internal/api/version_test.go @@ -0,0 +1,79 @@ +package api + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" +) + +func TestServerVersion(t *testing.T) { + t.Run("reports the image tag", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/version/" { + t.Errorf("path = %q", r.URL.Path) + } + fmt.Fprint(w, `{"ci_commit_sha":"67853f3","image_tag":"2.262.0","is_saas":true,"package_versions":{".":"2.262.0"}}`) + })) + defer srv.Close() + + // When + got, err := ServerVersion(context.Background(), srv.Client(), srv.URL+"/") + + // Then + if err != nil { + t.Fatal(err) + } + if got != "2.262.0" { + t.Errorf("ServerVersion = %q, want 2.262.0", got) + } + }) + + t.Run("errors when the endpoint is absent", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.NotFoundHandler()) + defer srv.Close() + + // When + _, err := ServerVersion(context.Background(), srv.Client(), srv.URL) + + // Then + if err == nil { + t.Error("err = nil, want an error") + } + }) + + t.Run("errors when the body is not the version document", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, "nope") + })) + defer srv.Close() + + // When + _, err := ServerVersion(context.Background(), srv.Client(), srv.URL) + + // Then + if err == nil { + t.Error("err = nil, want an error") + } + }) + + t.Run("errors when the tag is missing", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `{"ci_commit_sha":"67853f3"}`) + })) + defer srv.Close() + + // When + _, err := ServerVersion(context.Background(), srv.Client(), srv.URL) + + // Then + if err == nil { + t.Error("err = nil, want an error") + } + }) +} diff --git a/internal/auth/kind.go b/internal/auth/kind.go index 07eb758..809168b 100644 --- a/internal/auth/kind.go +++ b/internal/auth/kind.go @@ -20,15 +20,18 @@ const ( var legacyAuthtokenPattern = regexp.MustCompile(`^[0-9a-fA-F]{40}$`) -// ValidateMasterKey's rejections. +// ValidateMasterKey's rejections. Each reads as the predicate of a sentence +// whose subject is the variable the value came from, which only the caller +// knows: the Master API key variable is host-scoped, so naming it here would +// name the wrong one for every self-hosted instance. var ( - ErrServerSideKey = errors.New("FLAGSMITH_API_KEY contains a server-side environment key") - ErrLegacyAuthtoken = errors.New("FLAGSMITH_API_KEY contains a legacy user authtoken, which is not supported") - ErrNotMasterKey = errors.New("FLAGSMITH_API_KEY is not a Master API key (expected {prefix}.{secret})") + ErrServerSideKey = errors.New("holds a server-side environment key") + ErrLegacyAuthtoken = errors.New("holds a legacy user authtoken, which is not supported") + ErrNotMasterKey = errors.New("is not a Master API key (expected {prefix}.{secret})") ) -// ValidateMasterKey checks that a FLAGSMITH_API_KEY value is a Master API key. -// Each Admin API env var maps to exactly one credential kind, so the scheme is +// ValidateMasterKey checks that a credential value is a Master API key. Each +// Admin API env var maps to exactly one credential kind, so the scheme is // never guessed from token shape; this only turns common paste-mistakes into // actionable errors instead of a silently rejected request. func ValidateMasterKey(value string) error { diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index e6ff2b7..e52c7b4 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -127,6 +127,10 @@ func secureScheme(u *url.URL) bool { return ip != nil && ip.IsLoopback() } +// ErrNoDiscovery reports that an instance serves no authorization server +// metadata: a Flagsmith predating the OAuth login, or not a Flagsmith API. +var ErrNoDiscovery = errors.New("no authorization server metadata") + func Discover(ctx context.Context, httpClient *http.Client, apiURL string) (*Metadata, error) { u := strings.TrimRight(apiURL, "/") + "/.well-known/oauth-authorization-server" req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil) @@ -138,8 +142,14 @@ func Discover(ctx context.Context, httpClient *http.Client, apiURL string) (*Met return nil, bug.Mark(fmt.Errorf("reaching %s: %w", u, err)) } defer resp.Body.Close() + // Only a 404 means the document is absent. Any other refusal comes from a + // server that has one, and saying otherwise would send a user with a + // rate-limited or briefly broken instance looking for a URL mistake. + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("%s returned %s: %w", u, resp.Status, ErrNoDiscovery) + } if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%s returned %s — is this a Flagsmith API URL?", u, resp.Status) + return nil, bug.Mark(fmt.Errorf("%s returned %s", u, resp.Status)) } var md Metadata if err := json.NewDecoder(resp.Body).Decode(&md); err != nil { diff --git a/internal/auth/oauth_test.go b/internal/auth/oauth_test.go index 659c53b..0d566fe 100644 --- a/internal/auth/oauth_test.go +++ b/internal/auth/oauth_test.go @@ -17,6 +17,8 @@ import ( "sync" "testing" "time" + + "github.com/Flagsmith/flagsmith-cli/v2/internal/bug" ) // fakeAuthServer implements just enough of the Flagsmith OAuth 2.1 surface @@ -416,8 +418,31 @@ func TestDiscover(t *testing.T) { _, err := Discover(context.Background(), http.DefaultClient, srv.URL) // Then - if err == nil || !strings.Contains(err.Error(), "is this a Flagsmith API URL?") { - t.Errorf("err = %v, want a helpful not-Flagsmith hint", err) + if !errors.Is(err, ErrNoDiscovery) { + t.Errorf("err = %v, want ErrNoDiscovery", err) + } + }) + + // Only a 404 says the document is absent. A server that is failing or + // refusing has one, and callers act on the difference. + t.Run("a failing endpoint is not an absent one", func(t *testing.T) { + for _, status := range []int{http.StatusUnauthorized, http.StatusTooManyRequests, http.StatusInternalServerError} { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(status) + })) + + // When + _, err := Discover(context.Background(), http.DefaultClient, srv.URL) + srv.Close() + + // Then + if err == nil || errors.Is(err, ErrNoDiscovery) { + t.Errorf("%d: err = %v, want an error that is not ErrNoDiscovery", status, err) + } + if !errors.Is(err, bug.ErrUnexpected) { + t.Errorf("%d: err = %v, want it marked reportable", status, err) + } } }) diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index 91b2290..60d1eec 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "io" "strconv" @@ -86,7 +87,7 @@ func resolveCredential(ctx context.Context) (*activeCredential, error) { func loadCredential(ctx context.Context) (*activeCredential, error) { if name, v := envCredential(envAPIKey, apiURL, defaultAPIURL); v != "" { if err := auth.ValidateMasterKey(v); err != nil { - return nil, err + return nil, fmt.Errorf("%s %w", name, err) } cred := &activeCredential{kind: auth.KindMaster, token: v, source: "$" + name, auth: api.APIKey(v)} cred.apiClient = newAPIClient(cred.auth) @@ -100,6 +101,15 @@ func loadCredential(ctx context.Context) (*activeCredential, error) { } creds, err := auth.Load(apiURL) + if errors.Is(err, auth.ErrNotLoggedIn) { + // Reporting "not logged in" to someone who did set a credential sends + // them to do what they just did. Their key is fine; its name isn't. + if set, use := ignoredUnscopedCredential(); set != "" { + return nil, hintf(fmt.Errorf("%s is set, but ignored for %s", set, apiURL), + "Credentials are host-scoped away from %s — set %s instead.", + urlHost(defaultAPIURL), use) + } + } if err != nil { return nil, err } diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 0b98cd2..0efde2c 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -2159,6 +2160,44 @@ func TestConfigCommand(t *testing.T) { t.Errorf("err = %v (hint %q), want a hint pointing at FLAGSMITH_ENVIRONMENT_KEY", err, hintFor(err)) } }) + + // A bad config file fails resolveContext before it can publish either + // surface URL, so the hint has no instance to scope a name to. Every test + // in this process shares those globals, so clear them to model a real run, + // which is one command in a process of its own. + t.Run("a config that fails to load still names a usable variable", func(t *testing.T) { + // Given + isolateStorage(t) + writeConfig(t, tempRepo(t), `{"environment": "ser.SuperSecret123"}`) + defer func(a, s string) { apiURL, sdkAPIURL = a, s }(apiURL, sdkAPIURL) + apiURL, sdkAPIURL = "", "" + + // When + _, err := run("", "config") + + // Then + hint := hintFor(err) + if err == nil || !strings.Contains(hint, envEnvironmentKey+" ") { + t.Errorf("err = %v (hint %q), want the plain %s named", err, hint, envEnvironmentKey) + } + }) + + // The SDK credential is scoped to the SDK surface, so the variable to set + // is only knowable once that URL is resolved. + t.Run("server-side key rejection names the scoped variable", func(t *testing.T) { + // Given + isolateStorage(t) + tempRepo(t) + + // When + _, err := run("", "config", "-e", "ser.AbCd", "--sdk-api-url", "https://sdk.example.com") + + // Then + want := "FLAGSMITH_ENVIRONMENT_KEY_sdk_example_com" + if err == nil || !strings.Contains(hintFor(err), want) { + t.Errorf("err = %v (hint %q), want a hint pointing at %s", err, hintFor(err), want) + } + }) } // fakeTTY makes prompts believe stdin is a terminal for one test. @@ -7384,8 +7423,8 @@ func TestUnscopedCredentialNotSentToRedirectedHost(t *testing.T) { _, err := run("", "flag", "list") // Then - if !errors.Is(err, auth.ErrNotLoggedIn) { - t.Errorf("err = %v, want ErrNotLoggedIn (credential withheld)", err) + if err == nil || !strings.Contains(err.Error(), envAPIKey) { + t.Errorf("err = %v, want the credential withheld and said to be", err) } if got := f.featuresCalls(); got != 0 { t.Errorf("features calls = %d, want 0 — no request should carry the key", got) @@ -7423,8 +7462,8 @@ func TestUnscopedAccessTokenNotSentToRedirectedHost(t *testing.T) { _, err := run("", "auth", "status") // Then - if !errors.Is(err, auth.ErrNotLoggedIn) { - t.Errorf("err = %v, want ErrNotLoggedIn (bearer withheld)", err) + if err == nil || !strings.Contains(err.Error(), envAccessToken) { + t.Errorf("err = %v, want the bearer withheld and said to be", err) } if got := f.organisationLists(); got != 0 { t.Errorf("organisation calls = %d, want 0 — no request should carry the bearer", got) @@ -7559,6 +7598,11 @@ func TestEnvServerKeyRejected(t *testing.T) { if err == nil || !strings.Contains(hintFor(err), "FLAGSMITH_ENVIRONMENT_KEY") { t.Errorf("err = %v (hint %q), want a hint pointing at FLAGSMITH_ENVIRONMENT_KEY", err, hintFor(err)) } + // The rejection must name the variable the value was read from, which off + // the default host is never the unscoped one. + if want := scopedEnvName(envAPIKey, f.srv.URL); err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("err = %v, want it to name %s", err, want) + } } func TestEnvBeatsKeychain(t *testing.T) { @@ -7586,6 +7630,46 @@ func TestEnvBeatsKeychain(t *testing.T) { } } +// A credential set in the unscoped variable is ignored off the default host. +// Reporting that as "not logged in" sends the user to do what they just did. +func TestUnscopedCredentialIsReportedAsIgnored(t *testing.T) { + t.Run("names both the ignored variable and the one to set", func(t *testing.T) { + // Given + isolateStorage(t) + f := newFakeInstance(t) + t.Setenv(envAPIKey, masterKey) + + // When + _, err := run("", "project", "list", "--api-url", f.srv.URL) + + // Then + if err == nil { + t.Fatal("err = nil, want the ignored credential reported") + } + if !strings.Contains(err.Error(), envAPIKey) || strings.Contains(err.Error(), "not logged in") { + t.Errorf("err = %v, want it to name the ignored variable rather than report a login", err) + } + if want := scopedEnvName(envAPIKey, f.srv.URL); !strings.Contains(hintFor(err), want) { + t.Errorf("hint = %q, want it to name %s", hintFor(err), want) + } + }) + + t.Run("silent when the variable is scoped to this instance", func(t *testing.T) { + // Given + isolateStorage(t) + f := newFakeInstance(t) + setMasterKey(t, f.srv.URL) + + // When + _, err := run("", "project", "list", "--api-url", f.srv.URL) + + // Then + if err != nil { + t.Fatalf("project list: %v", err) + } + }) +} + func TestLoginFailsClosedWithoutKeychain(t *testing.T) { // Given isolateStorage(t) @@ -7607,6 +7691,88 @@ func TestLoginFailsClosedWithoutKeychain(t *testing.T) { } } +// oldInstance serves a /version/ document reporting tag, and nothing else — in +// particular no OAuth metadata, like a Flagsmith predating the browser login. +// An empty tag serves no version endpoint at all. +func oldInstance(t *testing.T, tag string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + if tag != "" { + mux.HandleFunc("GET /version/", func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, `{"image_tag":%q}`, tag) + }) + } + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + +// A Flagsmith too old to serve OAuth metadata has a way forward — a Master API +// key — that the bare 404 from the discovery endpoint does not mention. +func TestLoginAgainstAnInstanceWithoutOAuth(t *testing.T) { + t.Run("old version names the version and the key variable to set", func(t *testing.T) { + // Given + isolateStorage(t) + srv := oldInstance(t, "2.180.3") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + hint := hintFor(err) + if err == nil { + t.Fatal("err = nil, want a login failure") + } + for _, want := range []string{"2.180.3", minOAuthVersion, scopedEnvName(envAPIKey, srv.URL)} { + if !strings.Contains(hint, want) { + t.Errorf("hint = %q, want it to mention %q", hint, want) + } + } + }) + + t.Run("unparseable image tag falls back to the generic hint", func(t *testing.T) { + // Given: self-hosted images are routinely tagged like this + isolateStorage(t) + srv := oldInstance(t, "latest") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + if got := hintFor(err); got != hintAPIURL { + t.Errorf("hint = %q, want the generic %q", got, hintAPIURL) + } + }) + + t.Run("no version endpoint falls back to the generic hint", func(t *testing.T) { + // Given + isolateStorage(t) + srv := oldInstance(t, "") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + if got := hintFor(err); got != hintAPIURL { + t.Errorf("hint = %q, want the generic %q", got, hintAPIURL) + } + }) + + t.Run("current version falls back to the generic hint", func(t *testing.T) { + // Given: new enough for OAuth, so a missing document is not about age + isolateStorage(t) + srv := oldInstance(t, "2.262.0") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + if got := hintFor(err); got != hintAPIURL { + t.Errorf("hint = %q, want the generic %q", got, hintAPIURL) + } + }) +} + func TestRefreshPersistsToKeychain(t *testing.T) { // Given isolateStorage(t) diff --git a/internal/cmd/context.go b/internal/cmd/context.go index 949b0ca..79c8c9d 100644 --- a/internal/cmd/context.go +++ b/internal/cmd/context.go @@ -143,14 +143,10 @@ func resolveContext(cmd *cobra.Command) (*projectContext, error) { return file.Organisation.Value(), true }, nil) - // environment (client-side key; ser.* never belongs in context) + // environment (a client-side key; the ser.* guard is below, once the SDK + // surface is known and the variable to name with it) pc.Environment = contextValue(cmd, "environment", environmentFlag, "FLAGSMITH_ENVIRONMENT", asString, func() (any, bool) { return file.Environment, file.Environment != "" }, nil) - if key, ok := pc.Environment.Value.(string); ok && strings.HasPrefix(key, "ser.") { - return nil, withHint( - errors.New("the environment context takes a client-side key"), - hintServerSideKey) - } pc.APIURL = contextValue(cmd, "api-url", apiURLFlag, "FLAGSMITH_API_URL", trimSlash, func() (any, bool) { return strings.TrimRight(file.APIURL, "/"), file.APIURL != "" }, defaultAPIURL) @@ -163,6 +159,18 @@ func resolveContext(cmd *cobra.Command) (*projectContext, error) { pc.SDKAPIURL = contextValue(cmd, "sdk-api-url", sdkAPIURLFlag, "FLAGSMITH_SDK_API_URL", trimSlash, func() (any, bool) { return strings.TrimRight(file.SDKAPIURL, "/"), file.SDKAPIURL != "" }, sdkDefault) + // Both surfaces are now known; the credential layer and every hint naming a + // host-scoped variable read them from here. + apiURL = pc.apiURL() + sdkAPIURL, _ = pc.SDKAPIURL.Value.(string) + + // ser.* is a secret and never a context value. + if key, ok := pc.Environment.Value.(string); ok && strings.HasPrefix(key, "ser.") { + return nil, withHint( + errors.New("the environment context takes a client-side key"), + hintServerSideKey()) + } + // Cosmetic name enrichment from the local cache — never the network. names := cache.Load(pc.apiURL()) if id, ok := pc.Project.Value.(int); ok { @@ -187,6 +195,5 @@ func applyContext(cmd *cobra.Command) (*projectContext, error) { for _, w := range pc.Warnings { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", w) } - apiURL = pc.apiURL() return pc, nil } diff --git a/internal/cmd/envcred.go b/internal/cmd/envcred.go index 0589dbe..5c2edc5 100644 --- a/internal/cmd/envcred.go +++ b/internal/cmd/envcred.go @@ -28,9 +28,12 @@ func envBool(name string) bool { // scopedEnvName is the host-scoped form of a credential variable for an // instance URL: the host and port with `-` written `__` and `.` and `:` -// written `_`. The scheme is not part of the scope. +// written `_`. The scheme is not part of the scope. The brackets around an +// IPv6 literal are dropped rather than encoded — they are URL syntax, and a +// name carrying them is one no shell can export. func scopedEnvName(base, rawURL string) string { host := urlHost(rawURL) + host = strings.NewReplacer("[", "", "]", "").Replace(host) host = strings.ReplaceAll(host, "-", "__") host = strings.ReplaceAll(host, ".", "_") host = strings.ReplaceAll(host, ":", "_") @@ -63,6 +66,43 @@ func envCredential(base, rawURL, defaultURL string) (name, value string) { return "", "" } +// envVarFor names the variable a user should set to reach an instance — the +// one envCredential will actually read, which off the default host is only ever +// the host-scoped form. An instance that is not resolved yet gets the unscoped +// name: a hint can be rendered from an error raised before the surface URLs are +// known, and a scope suffixed to nothing names nothing. +func envVarFor(base, rawURL, defaultURL string) string { + host := urlHost(rawURL) + if host == "" || host == urlHost(defaultURL) { + return base + } + return scopedEnvName(base, rawURL) +} + +// ignoredUnscopedCredential names an unscoped credential variable that is set +// but cannot be read for this instance, together with the variable that would +// be. Both are "" when nothing is being ignored. It is only meaningful once +// scoped lookups have missed, which is the only place it is called from. +func ignoredUnscopedCredential() (set, use string) { + if urlHost(apiURL) == urlHost(defaultAPIURL) { + return "", "" + } + for _, base := range []string{envAPIKey, envAccessToken} { + if os.Getenv(base) != "" { + return base, scopedEnvName(base, apiURL) + } + } + return "", "" +} + +// apiKeyVar and accessTokenVar name the Admin API credential variables for the +// instance this invocation is talking to; environmentKeyVar names the SDK one, +// which scopes to the SDK surface — a host of its own, and by default not even +// the same one. +func apiKeyVar() string { return envVarFor(envAPIKey, apiURL, defaultAPIURL) } +func accessTokenVar() string { return envVarFor(envAccessToken, apiURL, defaultAPIURL) } +func environmentKeyVar() string { return envVarFor(envEnvironmentKey, sdkAPIURL, defaultSDKAPIURL) } + // lookupEnvFold finds an environment variable by case-insensitive name, // returning the name as actually set. Host-scoped variable names embed a // hostname, which is itself case-insensitive. diff --git a/internal/cmd/envcred_test.go b/internal/cmd/envcred_test.go index cb7e1e4..c6d6acb 100644 --- a/internal/cmd/envcred_test.go +++ b/internal/cmd/envcred_test.go @@ -15,6 +15,11 @@ func TestScopedEnvName(t *testing.T) { "http://localhost:8000": "FLAGSMITH_API_KEY_localhost_8000", "http://127.0.0.1:8000/api": "FLAGSMITH_API_KEY_127_0_0_1_8000", "https://Flagsmith.Example.COM": "FLAGSMITH_API_KEY_flagsmith_example_com", + // An IPv6 literal's brackets are URL syntax, and no shell can export a + // variable whose name carries them. + "http://[::1]:8000": "FLAGSMITH_API_KEY___1_8000", + "https://[2001:db8::1]:8443": "FLAGSMITH_API_KEY_2001_db8__1_8443", + "https://[2001:DB8::1]": "FLAGSMITH_API_KEY_2001_db8__1", } for in, want := range cases { if got := scopedEnvName(envAPIKey, in); got != want { @@ -23,6 +28,28 @@ func TestScopedEnvName(t *testing.T) { } } +// The variable a user should set is the one that will actually be read: the +// unscoped form only where it is trusted, the host-scoped form everywhere else. +func TestEnvVarFor(t *testing.T) { + cases := map[string]string{ + "https://api.flagsmith.com": envAPIKey, + "https://api.flagsmith.com/": envAPIKey, + "https://API.Flagsmith.com": envAPIKey, + "https://flagsmith.example.com": "FLAGSMITH_API_KEY_flagsmith_example_com", + "http://localhost:8000": "FLAGSMITH_API_KEY_localhost_8000", + // No instance resolved yet: a scoped name would be a bare suffix that + // names nothing and cannot be set. + "": envAPIKey, + "/": envAPIKey, + "///": envAPIKey, + } + for in, want := range cases { + if got := envVarFor(envAPIKey, in, defaultAPIURL); got != want { + t.Errorf("envVarFor(%q) = %q, want %q", in, got, want) + } + } +} + // Boolean switches read a value, not merely presence: FLAGSMITH_NO_INPUT=false // must not disable prompting. func TestEnvBool(t *testing.T) { diff --git a/internal/cmd/errors.go b/internal/cmd/errors.go index 78fdb3d..8256186 100644 --- a/internal/cmd/errors.go +++ b/internal/cmd/errors.go @@ -19,16 +19,12 @@ import ( const ( hintPricing = "This isn't available on your current plan — see https://flagsmith.com/pricing" hintQuota = "Enterprise plans can raise this limit — get in touch: https://docs.flagsmith.com/support#getting-in-touch" - hintLogin = "Run `flagsmith login`, or set FLAGSMITH_API_KEY for non-interactive use." - hintMasterKey = "Set FLAGSMITH_API_KEY to use a Master API key instead." hintMasterKeyOrLogin = "Use a Master API key, or run `flagsmith login`." hintRelogin = "Run `flagsmith login` to re-authenticate." - hintAccessToken = "For an OAuth access token, set FLAGSMITH_ACCESS_TOKEN instead." - hintServerSideKey = "Server-side keys are secrets — provide them via FLAGSMITH_ENVIRONMENT_KEY instead." - hintEnvironmentKey = "Check FLAGSMITH_ENVIRONMENT_KEY, or the environment name/key passed with -e." - hintSDKAPIURL = "Check --sdk-api-url (or `sdkApiUrl`) — it must point at a Flagsmith SDK API." + hintSDKAPIURL = "Check --sdk-api-url (or `sdkApiUrl`) — it must point at a Flagsmith SDK API." + hintAPIURL = "Check --api-url (or `apiUrl`) — it must point at a Flagsmith API." hintEnvironmentList = "Run `flagsmith environment list` to see the environments in this project." hintProjectList = "Run `flagsmith project list` to see the projects you can access." @@ -42,6 +38,28 @@ const ( hintReportIssue = "Think this shouldn't happen? Tell us: https://github.com/Flagsmith/flagsmith-cli/issues/new" ) +// Hints that name a credential variable are functions of the instance in play, +// because the name is: the unscoped form is read only for the default host. +func hintLogin() string { + return fmt.Sprintf("Run `flagsmith login`, or set %s for non-interactive use.", apiKeyVar()) +} + +func hintMasterKey() string { + return fmt.Sprintf("Set %s to use a Master API key instead.", apiKeyVar()) +} + +func hintAccessToken() string { + return fmt.Sprintf("For an OAuth access token, set %s instead.", accessTokenVar()) +} + +func hintServerSideKey() string { + return fmt.Sprintf("Server-side keys are secrets — provide them via %s instead.", environmentKeyVar()) +} + +func hintEnvironmentKey() string { + return fmt.Sprintf("Check %s, or the environment name/key passed with -e.", environmentKeyVar()) +} + // docsHint points at a page under docs.flagsmith.com. func docsHint(path string) string { return "See https://docs.flagsmith.com/" + path @@ -80,17 +98,19 @@ func hintFor(err error) string { } switch { case errors.Is(err, auth.ErrNotLoggedIn): - return hintLogin + return hintLogin() + case errors.Is(err, auth.ErrNoDiscovery): + return hintAPIURL case errors.Is(err, auth.ErrRefreshFailed): return hintRelogin case errors.Is(err, auth.ErrKeychainUnavailable): - return hintMasterKey + return hintMasterKey() case errors.Is(err, auth.ErrLegacyAuthtoken): return hintMasterKeyOrLogin case errors.Is(err, auth.ErrNotMasterKey): - return hintAccessToken + return hintAccessToken() case errors.Is(err, auth.ErrServerSideKey), errors.Is(err, config.ErrServerSideKey): - return hintServerSideKey + return hintServerSideKey() case errors.Is(err, api.ErrQuotaExceeded): return hintQuota case errors.Is(err, api.ErrPlanGated): diff --git a/internal/cmd/errors_test.go b/internal/cmd/errors_test.go index 20cfbf6..860140c 100644 --- a/internal/cmd/errors_test.go +++ b/internal/cmd/errors_test.go @@ -21,18 +21,20 @@ func TestHintFor(t *testing.T) { err error want string }{ - {"not logged in", auth.ErrNotLoggedIn, hintLogin}, + {"not logged in", auth.ErrNotLoggedIn, hintLogin()}, + {"no discovery document", auth.ErrNoDiscovery, hintAPIURL}, + {"no discovery document wrapped", fmt.Errorf("%s returned 404: %w", "https://x/.well-known", auth.ErrNoDiscovery), hintAPIURL}, {"plan gated", api.ErrPlanGated, hintPricing}, {"plan gated wrapped", fmt.Errorf("create project: %w", api.ErrPlanGated), hintPricing}, {"quota exceeded", api.ErrQuotaExceeded, hintQuota}, {"quota exceeded wrapped", fmt.Errorf("create segment: %w", api.ErrQuotaExceeded), hintQuota}, {"workflow gated", api.ErrWorkflowGated, docsHint("advanced-use/change-requests")}, - {"keychain unavailable", auth.ErrKeychainUnavailable, hintMasterKey}, + {"keychain unavailable", auth.ErrKeychainUnavailable, hintMasterKey()}, {"session refresh failed wrapped", fmt.Errorf("%w: %w", auth.ErrRefreshFailed, errors.New("boom")), hintRelogin}, - {"server-side key in FLAGSMITH_API_KEY", auth.ErrServerSideKey, hintServerSideKey}, + {"server-side key in FLAGSMITH_API_KEY", auth.ErrServerSideKey, hintServerSideKey()}, {"legacy authtoken", auth.ErrLegacyAuthtoken, hintMasterKeyOrLogin}, - {"not a master key", auth.ErrNotMasterKey, hintAccessToken}, - {"server-side key in config file", fmt.Errorf("flagsmith.json: %w", config.ErrServerSideKey), hintServerSideKey}, + {"not a master key", auth.ErrNotMasterKey, hintAccessToken()}, + {"server-side key in config file", fmt.Errorf("flagsmith.json: %w", config.ErrServerSideKey), hintServerSideKey()}, {"marked unexpected", bug.Mark(errors.New("boom")), hintReportIssue}, {"specific hint beats report-issue", bug.Mark(fmt.Errorf("%w: %w", auth.ErrRefreshFailed, errors.New("boom"))), hintRelogin}, {"explicit hint wins over automatic", withHint(api.ErrPlanGated, "custom"), "custom"}, @@ -49,6 +51,49 @@ func TestHintFor(t *testing.T) { } } +// Unscoped credential variables are read only for the default host, so a hint +// that names one off it sends a self-hosted user to set something that will be +// ignored. +func TestCredentialHintsNameTheVariableThatIsRead(t *testing.T) { + defer func(a, s string) { apiURL, sdkAPIURL = a, s }(apiURL, sdkAPIURL) + + // The SDK credential scopes to the SDK surface, which is its own host. + sdkAPIURL = "https://sdk.example.com" + for name, got := range map[string]string{ + "hintServerSideKey": hintServerSideKey(), + "hintEnvironmentKey": hintEnvironmentKey(), + } { + if want := "FLAGSMITH_ENVIRONMENT_KEY_sdk_example_com"; !strings.Contains(got, want) { + t.Errorf("%s() = %q, want it to name %s", name, got, want) + } + } + sdkAPIURL = defaultSDKAPIURL + if got := hintEnvironmentKey(); !strings.Contains(got, envEnvironmentKey+",") { + t.Errorf("hintEnvironmentKey() on the default SDK host = %q, want the unscoped variable", got) + } + + apiURL = "https://flagsmith.example.com" + scoped := map[string]string{ + "hintLogin": hintLogin(), + "hintMasterKey": hintMasterKey(), + "hintAccessToken": hintAccessToken(), + } + for name, got := range scoped { + want := "FLAGSMITH_API_KEY_flagsmith_example_com" + if name == "hintAccessToken" { + want = "FLAGSMITH_ACCESS_TOKEN_flagsmith_example_com" + } + if !strings.Contains(got, want) { + t.Errorf("%s() = %q, want it to name %s", name, got, want) + } + } + + apiURL = defaultAPIURL + if got := hintLogin(); !strings.Contains(got, envAPIKey+" ") { + t.Errorf("hintLogin() on the default host = %q, want the unscoped variable", got) + } +} + func TestReportError(t *testing.T) { newCmd := func(buf *bytes.Buffer) *cobra.Command { c := &cobra.Command{Use: "demo", Short: "demo"} diff --git a/internal/cmd/evaluate.go b/internal/cmd/evaluate.go index 66b0d2f..887968d 100644 --- a/internal/cmd/evaluate.go +++ b/internal/cmd/evaluate.go @@ -304,7 +304,7 @@ func evalError(sdkURL string, err error) error { return bug.Mark(fmt.Errorf("evaluating flags on %s failed", sdkURL)) case http.StatusUnauthorized, http.StatusForbidden: return withHint(fmt.Errorf("%s rejected the environment key (%s)", sdkURL, apiErr.ResponseStatus), - hintEnvironmentKey) + hintEnvironmentKey()) case http.StatusNotFound: return withHint(fmt.Errorf("%s has no SDK API to evaluate against (%s)", sdkURL, apiErr.ResponseStatus), hintSDKAPIURL) diff --git a/internal/cmd/init.go b/internal/cmd/init.go index a094690..72b045c 100644 --- a/internal/cmd/init.go +++ b/internal/cmd/init.go @@ -137,8 +137,9 @@ func runInit(cmd *cobra.Command, args []string) error { cred, err := resolveCredential(ctx) if errors.Is(err, auth.ErrNotLoggedIn) { if !interactive() { - return withHint(errors.New("no credentials found, and a browser login needs a TTY"), - "Set FLAGSMITH_API_KEY, run in a CI OIDC context with an org trust relationship, or run `flagsmith login` interactively first.") + return hintf(errors.New("no credentials found, and a browser login needs a TTY"), + "Set %s, run in a CI OIDC context with an org trust relationship, or run `flagsmith login` interactively first.", + apiKeyVar()) } if err := browserLogin(cmd); err != nil { return err diff --git a/internal/cmd/login.go b/internal/cmd/login.go index c383a5a..0bcbbc5 100644 --- a/internal/cmd/login.go +++ b/internal/cmd/login.go @@ -1,9 +1,11 @@ package cmd import ( + "context" "errors" "fmt" + "github.com/blang/semver/v4" "github.com/pkg/browser" "github.com/spf13/cobra" @@ -12,6 +14,10 @@ import ( "github.com/Flagsmith/flagsmith-cli/v2/internal/output" ) +// minOAuthVersion is the Flagsmith release that added the admin-api OAuth +// scope the CLI logs in with: https://github.com/Flagsmith/flagsmith/releases/tag/v2.255.0 +const minOAuthVersion = "2.255.0" + var noBrowser bool func newLoginCmd() *cobra.Command { @@ -37,19 +43,40 @@ func newLoginCmd() *cobra.Command { return cmd } +// tooOldForOAuthHint explains a missing discovery document when the instance +// turns out to be a Flagsmith predating the OAuth login, and reports "" — no +// hint, leaving the generic one — whenever that cannot be established. Only a +// self-hosted instance can be this old, so the Master API key it points at is +// named host-scoped. An instance reporting an image tag that is not a version +// ("latest", a commit sha) says nothing about its age: claiming it is too old +// would send a user with a healthy instance chasing an upgrade they don't need. +func tooOldForOAuthHint(ctx context.Context) string { + tag, err := api.ServerVersion(ctx, sharedHTTPClient(), apiURL) + if err != nil { + return "" + } + got, err := semver.ParseTolerant(tag) + if err != nil || got.GTE(semver.MustParse(minOAuthVersion)) { + return "" + } + return fmt.Sprintf( + "This instance runs Flagsmith %s; browser login needs %s or newer. Upgrade it, or set %s to a Master API key.", + tag, minOAuthVersion, apiKeyVar()) +} + func browserLogin(cmd *cobra.Command) error { // --no-input promises zero interaction; a browser login is nothing but // interaction. Master API keys go through FLAGSMITH_API_KEY. (--yes is // authorization, not a liveness switch, so it does not block login.) if noInput() { return withHint(errors.New("browser login needs a terminal and cannot run with --no-input"), - hintMasterKey) + hintMasterKey()) } // The session lives in the OS keychain; without one, minting tokens we // can't store would strand a live session — fail closed toward the env var. if !auth.KeychainAvailable() { return withHint(errors.New("no OS keychain available to store the session"), - hintMasterKey) + hintMasterKey()) } open := browser.OpenURL if noBrowser || !stdinIsTTY() { @@ -57,6 +84,9 @@ func browserLogin(cmd *cobra.Command) error { } creds, err := auth.Login(cmd.Context(), sharedHTTPClient(), apiURL, open, cmd.OutOrStdout()) if err != nil { + if errors.Is(err, auth.ErrNoDiscovery) { + return withHint(err, tooOldForOAuthHint(cmd.Context())) + } return err } if err := auth.Save(creds); err != nil { diff --git a/internal/cmd/resolve.go b/internal/cmd/resolve.go index 00bc6db..363ec2a 100644 --- a/internal/cmd/resolve.go +++ b/internal/cmd/resolve.go @@ -82,13 +82,14 @@ func resolveEnvironment(cmd *cobra.Command, pc *projectContext, cred *activeCred // The SDK credential doubles as an environment reference, host-scoped // to the SDK surface. sdkURL, _ := pc.SDKAPIURL.Value.(string) - _, ref = envCredential(envEnvironmentKey, sdkURL, defaultSDKAPIURL) + name, r := envCredential(envEnvironmentKey, sdkURL, defaultSDKAPIURL) + ref = r // Server-side keys belong in that variable but can never resolve an // environment over the Admin API. Name the variable, never its value: // it is a secret and must stay out of stderr and CI logs. if strings.HasPrefix(ref, "ser.") { return api.Environment{}, withHint( - errors.New("FLAGSMITH_ENVIRONMENT_KEY holds a server-side key, which cannot identify an environment for Admin commands"), + fmt.Errorf("%s holds a server-side key, which cannot identify an environment for Admin commands", name), "Pass -e, set FLAGSMITH_ENVIRONMENT, or run `flagsmith init`.") } } @@ -126,8 +127,8 @@ func sdkEnvironmentKey(cmd *cobra.Command, pc *projectContext) (string, error) { } ref, _ := pc.Environment.Value.(string) if ref == "" { - return "", withHint(errors.New("no environment key"), - "Set FLAGSMITH_ENVIRONMENT_KEY, or pass -e.") + return "", hintf(errors.New("no environment key"), + "Set %s, or pass -e.", environmentKeyVar()) } names := cache.Load(pc.apiURL()).Environments if _, cached := names[ref]; cached { @@ -165,7 +166,7 @@ func resolveEnvironmentRef(cmd *cobra.Command, cred *activeCredential, projectID if strings.HasPrefix(ref, "ser.") { return nil, withHint( errors.New("the environment reference takes a client-side key, not a server-side one"), - hintServerSideKey) + hintServerSideKey()) } envs, err := cred.client().Environments(cmd.Context(), projectID) if err != nil { diff --git a/internal/cmd/root.go b/internal/cmd/root.go index b5501b1..82b6e14 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -76,8 +76,12 @@ func singleLineUsage(template string) string { return strings.Replace(template, defaultBlock, oneLine, 1) } -// apiURL is the resolved instance URL for the current invocation. -var apiURL string +// apiURL and sdkAPIURL are the resolved Admin and SDK surface URLs for the +// current invocation, set by resolveContext as soon as each is known. +var ( + apiURL string + sdkAPIURL string +) var ( apiURLFlag string diff --git a/internal/cmd/sentinel_test.go b/internal/cmd/sentinel_test.go index 2a5db67..375bcb4 100644 --- a/internal/cmd/sentinel_test.go +++ b/internal/cmd/sentinel_test.go @@ -26,6 +26,7 @@ func TestEverySentinelHasAHintDecision(t *testing.T) { "api.ErrQuotaExceeded": api.ErrQuotaExceeded, "api.ErrWorkflowGated": api.ErrWorkflowGated, "auth.ErrNotLoggedIn": auth.ErrNotLoggedIn, + "auth.ErrNoDiscovery": auth.ErrNoDiscovery, "auth.ErrKeychainUnavailable": auth.ErrKeychainUnavailable, "auth.ErrRefreshFailed": auth.ErrRefreshFailed, "auth.ErrServerSideKey": auth.ErrServerSideKey,