diff --git a/pkg/cmd/listen.go b/pkg/cmd/listen.go index f77f22d..497dc46 100644 --- a/pkg/cmd/listen.go +++ b/pkg/cmd/listen.go @@ -23,6 +23,7 @@ import ( "strconv" "strings" + "github.com/hookdeck/hookdeck-cli/pkg/ansi" "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" "github.com/hookdeck/hookdeck-cli/pkg/listen" "github.com/spf13/cobra" @@ -42,6 +43,72 @@ type listenCmd struct { filterPath string } +// applyCliKey resolves the project context for a --cli-key supplied on the +// command line, and saves the key when this machine has no stored credential. +// +// A key given on the command line determines its own project. Any project read +// from the config file belongs to a different login, and sending it alongside +// this key is what previously produced "your API key is invalid or expired" for +// anyone who already had a profile. Validation is project-agnostic (see +// Client.clientForCLIAuthValidate), so it resolves the project the key really +// belongs to, and that replaces whatever was on disk for this process. +// +// Saving is separate, and only happens when there is no stored credential. That +// covers the Hookdeck Console path, where the Console hands you a +// `listen ... --cli-key ` command and the key would otherwise be needed on +// every later run. When a credential already exists it is left alone: someone +// forwarding a Console source for a few minutes should not silently lose the +// login they had. +// +// The key is validated before anything is written, so a typo fails here with a +// clear error rather than being persisted and confusing the next run. +func (lc *listenCmd) applyCliKey(cmd *cobra.Command) error { + flag := cmd.Flags().Lookup("cli-key") + if flag == nil || !flag.Changed { + return nil + } + + // `--cli-key=` passes the Changed check but leaves nothing to authenticate + // with. Without this the empty value falls through InitConfig's coalesce + // and the run fails with "your API key is invalid or expired", which + // describes neither what happened nor how to fix it. Read the flag rather + // than Profile.APIKey, which by now may hold a stored key instead. + if strings.TrimSpace(flag.Value.String()) == "" { + return errors.New("--cli-key needs a value, e.g. --cli-key ") + } + + response, err := Config.GetAPIClient().ValidateAPIKey() + if err != nil { + return err + } + + // Adopt the key's own project, discarding any stale project from config. + Config.Profile.ApplyValidateAPIKeyResponse(response, true) + Config.RefreshCachedAPIClient() + + if Config.HasStoredAPIKey { + // Use the key for this run only; the existing login stays on disk. + return nil + } + + if err := Config.Profile.SaveProfile(); err != nil { + return err + } + if err := Config.Profile.UseProfile(); err != nil { + return err + } + Config.RefreshCachedAPIClient() + + // Writing credentials is a side effect of a command that otherwise only + // forwards events, so say so rather than doing it silently. + fmt.Printf( + "Saved CLI key for %s. Future runs won't need --cli-key.\n", + ansi.Bold(response.ProjectName), + ) + + return nil +} + // Map --cli-path to --path func normalizeCliPathFlag(f *pflag.FlagSet, name string) pflag.NormalizedName { switch name { @@ -162,6 +229,14 @@ Destination CLI path will be "/". To set the CLI path, use the "--path" flag.`, lc.cmd.Flags().BoolVar(&lc.noWSS, "no-wss", false, "Force unencrypted ws:// protocol instead of wss://") lc.cmd.Flags().MarkHidden("no-wss") + // Declared locally as well as on the root command. The root flag is hidden + // and deprecated, but `listen --cli-key` is a documented, supported way to + // authenticate a single run (from the Hookdeck Console, or in CI), and + // listen's own help promotes it. Binding to the same Config field keeps the + // behaviour identical; this only makes the flag discoverable in + // `hookdeck listen --help`. + lc.cmd.Flags().StringVar(&Config.Profile.APIKey, "cli-key", "", "Hookdeck CLI key used to authenticate this command, e.g. the key shown in the Hookdeck Console") + lc.cmd.Flags().StringVar(&lc.path, "path", "", "Sets the path to which events are forwarded e.g., /webhooks or /api/stripe") lc.cmd.Flags().IntVar(&lc.maxConnections, "max-connections", 50, "Maximum concurrent connections to local endpoint (default: 50, increase for high-volume testing)") @@ -241,6 +316,10 @@ Examples: // listenCmd represents the listen command func (lc *listenCmd) runListenCmd(cmd *cobra.Command, args []string) error { + if err := lc.applyCliKey(cmd); err != nil { + return err + } + var sourceQuery, connectionQuery string if len(args) > 1 { sourceQuery = args[1] diff --git a/pkg/cmd/listen_cli_key_test.go b/pkg/cmd/listen_cli_key_test.go new file mode 100644 index 0000000..b5c6aa2 --- /dev/null +++ b/pkg/cmd/listen_cli_key_test.go @@ -0,0 +1,195 @@ +package cmd + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/hookdeck/hookdeck-cli/pkg/config" + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// validateStub serves the project-agnostic cli-auth/validate endpoint, which is +// what resolves the project a supplied key belongs to. +func validateStub(t *testing.T, projectID, projectName string) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != hookdeck.APIPathPrefix+"/cli-auth/validate" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(hookdeck.ValidateAPIKeyResponse{ + ProjectID: projectID, + ProjectMode: "console", + ProjectName: projectName, + }) + })) + t.Cleanup(server.Close) + return server +} + +func listenCmdWithCliKeyFlag(t *testing.T, key string) *listenCmd { + t.Helper() + lc := newListenCmd() + require.NoError(t, lc.cmd.Flags().Set("cli-key", key)) + return lc +} + +func TestApplyCliKey(t *testing.T) { + t.Run("no-op when --cli-key was not supplied", func(t *testing.T) { + old := Config + t.Cleanup(func() { Config = old }) + config.ResetAPIClientForTesting() + t.Cleanup(config.ResetAPIClientForTesting) + + // Points at a port nothing is listening on: if the guard is wrong and a + // validate call is attempted, this fails rather than passing quietly. + Config = config.Config{} + Config.APIBaseURL = "http://127.0.0.1:1" + Config.Profile.ProjectId = "existing_project" + + lc := newListenCmd() + require.NoError(t, lc.applyCliKey(lc.cmd)) + assert.Equal(t, "existing_project", Config.Profile.ProjectId, + "context must be untouched when the flag is absent") + }) + + // The bug this guards against: a key given on the command line was sent + // alongside a project id read from a previous login, which belongs to a + // different project, so every call failed with "invalid or expired". + t.Run("adopts the key's own project over a stale stored one", func(t *testing.T) { + old := Config + t.Cleanup(func() { Config = old }) + config.ResetAPIClientForTesting() + t.Cleanup(config.ResetAPIClientForTesting) + + server := validateStub(t, "project_from_key", "Sandbox") + + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + require.NoError(t, os.WriteFile(path, []byte(`profile = "default" + +[default] +api_key = "sk_test_stored_key_1234" +project_id = "stale_project_from_previous_login" +`), 0600)) + + cfg, err := config.LoadConfigFromFile(path) + require.NoError(t, err) + cfg.APIBaseURL = server.URL + Config = *cfg + + lc := listenCmdWithCliKeyFlag(t, "sk_test_flag_key_5678") + Config.Profile.APIKey = "sk_test_flag_key_5678" + + require.NoError(t, lc.applyCliKey(lc.cmd)) + + assert.Equal(t, "project_from_key", Config.Profile.ProjectId, + "the key's project must replace the stale one for this run") + }) + + // A short debugging session with a Console key should not cost someone the + // login they already had. + t.Run("does not overwrite an existing stored login", func(t *testing.T) { + old := Config + t.Cleanup(func() { Config = old }) + config.ResetAPIClientForTesting() + t.Cleanup(config.ResetAPIClientForTesting) + + server := validateStub(t, "project_from_key", "Sandbox") + + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + stored := `profile = "default" + +[default] +api_key = "sk_test_stored_key_1234" +project_id = "stored_project" +` + require.NoError(t, os.WriteFile(path, []byte(stored), 0600)) + + cfg, err := config.LoadConfigFromFile(path) + require.NoError(t, err) + cfg.APIBaseURL = server.URL + Config = *cfg + require.True(t, Config.HasStoredAPIKey, "fixture must represent an existing login") + + lc := listenCmdWithCliKeyFlag(t, "sk_test_flag_key_5678") + Config.Profile.APIKey = "sk_test_flag_key_5678" + + require.NoError(t, lc.applyCliKey(lc.cmd)) + + after, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, stored, string(after), "an existing login must be left on disk untouched") + }) + + // `--cli-key=` satisfies Changed but carries nothing to authenticate with. + // It must fail locally rather than as an opaque auth error from the API. + t.Run("rejects an explicitly empty --cli-key before calling the API", func(t *testing.T) { + old := Config + t.Cleanup(func() { Config = old }) + config.ResetAPIClientForTesting() + t.Cleanup(config.ResetAPIClientForTesting) + + // A dead port: reaching the network at all fails this test. + Config = config.Config{} + Config.APIBaseURL = "http://127.0.0.1:1" + + lc := listenCmdWithCliKeyFlag(t, "") + + err := lc.applyCliKey(lc.cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "--cli-key needs a value", + "the error must name the flag, not surface as an auth failure") + }) + + t.Run("propagates a validation failure without writing", func(t *testing.T) { + old := Config + t.Cleanup(func() { Config = old }) + config.ResetAPIClientForTesting() + t.Cleanup(config.ResetAPIClientForTesting) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"message":"invalid api key"}`)) + })) + t.Cleanup(server.Close) + + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + require.NoError(t, os.WriteFile(path, []byte("profile = \"default\"\n"), 0600)) + + cfg, err := config.LoadConfigFromFile(path) + require.NoError(t, err) + cfg.APIBaseURL = server.URL + Config = *cfg + + lc := listenCmdWithCliKeyFlag(t, "sk_test_bogus_key_9999") + Config.Profile.APIKey = "sk_test_bogus_key_9999" + + require.Error(t, lc.applyCliKey(lc.cmd), "a key that fails validation must not be adopted") + + after, err := os.ReadFile(path) + require.NoError(t, err) + assert.NotContains(t, string(after), "sk_test_bogus_key_9999", + "an unvalidated key must never reach disk") + }) +} + +// The flag has to be declared on listen itself: the root declaration is hidden, +// so it appears in no help output and tooling that introspects the CLI cannot +// discover it. +func TestListenDeclaresCliKeyFlag(t *testing.T) { + lc := newListenCmd() + + flag := lc.cmd.Flags().Lookup("cli-key") + require.NotNil(t, flag, "listen must declare --cli-key") + assert.False(t, flag.Hidden, "--cli-key must be visible in `hookdeck listen --help`") +} diff --git a/pkg/config/config.go b/pkg/config/config.go index f57d26a..5b04102 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -42,6 +42,13 @@ type Config struct { configFile string // resolved path of config file viper *viper.Viper + // HasStoredAPIKey reports whether the config file already held a key when + // InitConfig ran, before any --cli-key/--api-key flag was folded in. + // Commands use it to tell "this machine already has a login" apart from + // "the key came from this invocation's flag", which the coalesced + // Profile.APIKey can no longer distinguish. + HasStoredAPIKey bool + // Telemetry TelemetryDisabled bool @@ -338,6 +345,11 @@ func (c *Config) constructConfig() { // "workspace" > "team" // TODO: use "project" instead of "workspace" // TODO: use "cli_key" instead of "api_key" + // Record whether a key was already on disk before the flag value wins the + // coalesce below, so commands can distinguish an existing login from a + // key supplied for this run only. + c.HasStoredAPIKey = stringCoalesce(c.viper.GetString(c.Profile.getConfigField("api_key")), c.viper.GetString("api_key"), "") != "" + c.Profile.APIKey = stringCoalesce(c.Profile.APIKey, c.viper.GetString(c.Profile.getConfigField("api_key")), c.viper.GetString("api_key"), "") c.Profile.ProjectId = stringCoalesce(c.Profile.ProjectId, c.viper.GetString(c.Profile.getConfigField("project_id")), c.viper.GetString("project_id"), c.viper.GetString(c.Profile.getConfigField("workspace_id")), c.viper.GetString(c.Profile.getConfigField("team_id")), c.viper.GetString("workspace_id"), "") diff --git a/pkg/config/has_stored_api_key_test.go b/pkg/config/has_stored_api_key_test.go new file mode 100644 index 0000000..1b0b82b --- /dev/null +++ b/pkg/config/has_stored_api_key_test.go @@ -0,0 +1,74 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// HasStoredAPIKey exists so commands can tell "this machine already has a +// login" apart from "a key was supplied for this run". Profile.APIKey cannot +// answer that on its own, because InitConfig coalesces the flag value over the +// stored one. +func TestHasStoredAPIKey(t *testing.T) { + t.Parallel() + + t.Run("false when the config file holds no key", func(t *testing.T) { + t.Parallel() + + c := Config{ + LogLevel: "info", + ConfigFileFlag: "./testdata/empty.toml", + } + c.InitConfig() + + assert.False(t, c.HasStoredAPIKey) + }) + + t.Run("true when the config file holds a key", func(t *testing.T) { + t.Parallel() + + c := Config{ + LogLevel: "info", + ConfigFileFlag: "./testdata/default-profile.toml", + } + c.InitConfig() + + assert.True(t, c.HasStoredAPIKey) + assert.Equal(t, "test_api_key", c.Profile.APIKey) + }) + + // The case the flag guard depends on: a key arriving by flag must not look + // like an existing login, or `listen --cli-key` would decline to save on a + // machine that has never been authenticated. + t.Run("false when the key came from a flag and none was stored", func(t *testing.T) { + t.Parallel() + + c := Config{ + LogLevel: "info", + ConfigFileFlag: "./testdata/empty.toml", + } + // Mirrors the flag binding: --cli-key writes here before InitConfig runs. + c.Profile.APIKey = "key_from_flag" + c.InitConfig() + + assert.False(t, c.HasStoredAPIKey, "a flag-supplied key is not a stored login") + assert.Equal(t, "key_from_flag", c.Profile.APIKey) + }) + + // And the converse: a flag value wins the coalesce, so Profile.APIKey alone + // can no longer reveal that a different key is still on disk. + t.Run("true when a flag overrides a stored key", func(t *testing.T) { + t.Parallel() + + c := Config{ + LogLevel: "info", + ConfigFileFlag: "./testdata/default-profile.toml", + } + c.Profile.APIKey = "key_from_flag" + c.InitConfig() + + assert.True(t, c.HasStoredAPIKey, "a stored login is still present behind the flag") + assert.Equal(t, "key_from_flag", c.Profile.APIKey, "the flag value wins for this run") + }) +}