diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 287e05f..5db60fa 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -10,7 +10,6 @@ on: merge_group: permissions: - id-token: write contents: read jobs: @@ -26,12 +25,7 @@ jobs: - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 with: go-version-file: go.mod - # setup-go caches by default; disable it here because this workflow - # also runs on tags and has id-token: write. cache: false - - name: OIDC Setup for goproxy - uses: github/setup-goproxy@5e60e1074d42316dfe2949ebf9a92bf77b24645b # v1.1.0 - - name: Run Go linter run: make lint diff --git a/README.md b/README.md index de9476d..9d0ddba 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,7 @@ gh elm --help # list available commands gh elm --version # print the extension version gh elm config # interactively set up credentials +gh elm config check # check source and target connectivity and authentication gh elm config show # print current config (tokens redacted) gh elm config reset # remove stored config and credentials gh elm config set-source-pat ORG # set an organization's SOURCE_PAT secret @@ -232,6 +233,13 @@ Where values are stored: Force a specific backend with `GH_ELM_CREDENTIAL_STORE=keyring` or `GH_ELM_CREDENTIAL_STORE=file`. `gh elm config show` prints which backend is active. +`gh elm config check` checks source and target independently. For each endpoint it reports +network reachability, API service health (including `5xx` responses), and whether the configured +credentials can access the authenticated-user endpoint. Results, including successes, are written +to standard error so they remain available when standard output is redirected or formatted as JSON. +Every source-backed `gh elm migration` command runs the same source check before its migration API +request. + ### Environment variables and precedence Every command resolves each URL and token in this order, so scripts and CI can skip diff --git a/integration/cli_test.go b/integration/cli_test.go index 1a74379..2d08c1f 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -69,8 +69,12 @@ func TestMigrationStatus(t *testing.T) { requestCount.Add(1) assert.Equal(t, http.MethodGet, r.Method) - assert.Equal(t, "/api/v3/enterprise/live-migrations/mig-1", r.URL.Path) assert.Equal(t, "Bearer source-token", r.Header.Get("Authorization")) + if r.URL.Path == "/api/v3/user" { + w.WriteHeader(http.StatusOK) + return + } + assert.Equal(t, "/api/v3/enterprise/live-migrations/mig-1", r.URL.Path) w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(response)) @@ -93,8 +97,10 @@ func TestMigrationStatus(t *testing.T) { for _, want := range []string{"Migration", "mig-1", "In progress"} { assert.Contains(t, result.Stdout, want) } - assert.Empty(t, result.Stderr) - assert.Equal(t, int32(1), requestCount.Load()) + assert.Contains(t, result.Stderr, "[OK] Source network") + assert.Contains(t, result.Stderr, "[OK] Source service") + assert.Contains(t, result.Stderr, "[OK] Source authentication") + assert.Equal(t, int32(2), requestCount.Load()) // Tokens must never be included in user-facing output. assert.NotContains(t, result.Stdout, "source-token") @@ -107,7 +113,7 @@ func TestMigrationStatus(t *testing.T) { requestCount.Add(1) assert.Equal(t, http.MethodGet, r.Method) - assert.Equal(t, "/api/v3/enterprise/live-migrations/mig-1", r.URL.Path) + assert.Equal(t, "/api/v3/user", r.URL.Path) assert.Equal(t, "Bearer invalid-source-token", r.Header.Get("Authorization")) w.Header().Set("Content-Type", "application/json") @@ -133,10 +139,10 @@ func TestMigrationStatus(t *testing.T) { assert.Equal(t, int32(1), requestCount.Load()) assert.Contains(t, result.Stderr, "Error\n") - assert.Contains(t, result.Stderr, "authentication failed") - assert.Contains(t, result.Stderr, "HTTP 401") - assert.Contains(t, result.Stderr, "GH_SOURCE_HOST") - assert.Contains(t, result.Stderr, "GH_SOURCE_TOKEN") + assert.Contains(t, result.Stderr, "[OK] Source network: reachable (HTTP 401 Unauthorized)") + assert.Contains(t, result.Stderr, "[OK] Source service: responding (HTTP 401 Unauthorized)") + assert.Contains(t, result.Stderr, "[FAIL] Source authentication: HTTP 401 Unauthorized") + assert.Contains(t, result.Stderr, "configured token was rejected or has expired") // The supplied credential must not be echoed in diagnostics. assert.NotContains(t, result.Stdout, "invalid-source-token") @@ -256,7 +262,7 @@ func TestSourceConfigurationPrecedence(t *testing.T) { require.Equal(t, 0, result.ExitCode, result.Stderr) assert.Contains(t, result.Stdout, `"source": "stored"`) - assert.Empty(t, result.Stderr) + assert.Contains(t, result.Stderr, "[OK] Source authentication") }) t.Run("environment overrides stored configuration", func(t *testing.T) { @@ -275,7 +281,7 @@ func TestSourceConfigurationPrecedence(t *testing.T) { require.Equal(t, 0, result.ExitCode, result.Stderr) assert.Contains(t, result.Stdout, `"source": "environment"`) - assert.Empty(t, result.Stderr) + assert.Contains(t, result.Stderr, "[OK] Source authentication") }) t.Run("flags override environment and stored configuration", func(t *testing.T) { @@ -298,12 +304,12 @@ func TestSourceConfigurationPrecedence(t *testing.T) { require.Equal(t, 0, result.ExitCode, result.Stderr) assert.Contains(t, result.Stdout, `"source": "flag"`) - assert.Empty(t, result.Stderr) + assert.Contains(t, result.Stderr, "[OK] Source authentication") }) - assert.Equal(t, int32(1), storedRequests.Load()) - assert.Equal(t, int32(1), envRequests.Load()) - assert.Equal(t, int32(1), flagRequests.Load()) + assert.Equal(t, int32(2), storedRequests.Load()) + assert.Equal(t, int32(2), envRequests.Load()) + assert.Equal(t, int32(2), flagRequests.Load()) } // newStatusServer returns a source API server that identifies itself in its @@ -318,8 +324,12 @@ func newStatusServer( requestCount.Add(1) assert.Equal(t, http.MethodGet, r.Method) - assert.Equal(t, "/api/v3/enterprise/live-migrations/mig-1", r.URL.Path) assert.Equal(t, "Bearer "+expectedToken, r.Header.Get("Authorization")) + if r.URL.Path == "/api/v3/user" { + w.WriteHeader(http.StatusOK) + return + } + assert.Equal(t, "/api/v3/enterprise/live-migrations/mig-1", r.URL.Path) w.Header().Set("Content-Type", "application/json") _, _ = fmt.Fprintf( diff --git a/internal/cmd/config_check.go b/internal/cmd/config_check.go new file mode 100644 index 0000000..6a25de5 --- /dev/null +++ b/internal/cmd/config_check.go @@ -0,0 +1,48 @@ +package cmd + +import ( + "errors" + "fmt" + "io" + + "github.com/spf13/cobra" + + "github.com/github/gh-elm/internal/endpoints" + "github.com/github/gh-elm/internal/preflight" +) + +func newConfigCheckCmd() *cobra.Command { + return &cobra.Command{ + Use: "check", + Short: "Check source and target connectivity and authentication", + Long: "Check network reachability, API service health, and authenticated-user access\n" + + "for both the configured source and target. All results are written to stderr.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + stderr := cmd.ErrOrStderr() + resolver, err := endpoints.NewResolver() + if err != nil { + sourceErr := checkResolvedEndpoint(cmd, stderr, "Source", "", "", err) + targetErr := checkResolvedEndpoint(cmd, stderr, "Target", "", "", err) + return errors.Join(sourceErr, targetErr) + } + + source, sourceResolveErr := resolver.Source("", "") + target, targetResolveErr := resolver.Target("", "") + sourceErr := checkResolvedEndpoint(cmd, stderr, "Source", source.URL, source.Token, sourceResolveErr) + targetErr := checkResolvedEndpoint(cmd, stderr, "Target", target.URL, target.Token, targetResolveErr) + return errors.Join(sourceErr, targetErr) + }, + } +} + +func checkResolvedEndpoint(cmd *cobra.Command, stderr io.Writer, name, endpointURL, token string, resolveErr error) error { + if resolveErr == nil { + return preflight.CheckEndpoint(cmd.Context(), stderr, name, endpointURL, token) + } + + fmt.Fprintf(stderr, "[FAIL] %s network: not checked because configuration could not be read\n", name) + fmt.Fprintf(stderr, "[FAIL] %s service: not checked because configuration could not be read\n", name) + fmt.Fprintf(stderr, "[FAIL] %s authentication: not checked because configuration could not be read\n", name) + return fmt.Errorf("%s preflight failed: reading configuration: %w", name, resolveErr) +} diff --git a/internal/cmd/configure.go b/internal/cmd/configure.go index 900d904..06d18dd 100644 --- a/internal/cmd/configure.go +++ b/internal/cmd/configure.go @@ -66,6 +66,7 @@ func newConfigCmd() *cobra.Command { _ = cmd.Flags().MarkHidden("show") _ = cmd.Flags().MarkHidden("reset") cmd.AddCommand( + newConfigCheckCmd(), newConfigShowCmd(), newConfigResetCmd(), newSetMigratorPATInteractiveCmd(), diff --git a/internal/cmd/configure_test.go b/internal/cmd/configure_test.go index 8cde35d..2647188 100644 --- a/internal/cmd/configure_test.go +++ b/internal/cmd/configure_test.go @@ -3,6 +3,11 @@ package cmd import ( "bytes" "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" "strings" "testing" @@ -71,6 +76,77 @@ func TestConfigureShow(t *testing.T) { assert.Contains(t, out, "not set", "unset target token should read as not set") } +func TestConfigCheck(t *testing.T) { + assertBothEndpointsReported := func(t *testing.T) { + t.Helper() + + output, err := execConfigure("check") + + require.Error(t, err) + for _, endpoint := range []string{"Source", "Target"} { + for _, check := range []string{"network", "service", "authentication"} { + assert.Contains(t, output, fmt.Sprintf("[FAIL] %s %s: not checked because configuration could not be read", endpoint, check)) + } + } + } + + t.Run("checks configured endpoints", func(t *testing.T) { + seedFileStore(t) + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Contains(t, []string{"/api/v3/meta", "/api/v3/user"}, r.URL.Path) + w.WriteHeader(http.StatusOK) + })) + defer source.Close() + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Contains(t, []string{"/meta", "/user"}, r.URL.Path) + if r.URL.Path == "/user" { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + require.NoError(t, (&config.Config{SourceURL: source.URL, TargetURL: target.URL}).Save()) + store, err := creds.NewStore() + require.NoError(t, err) + require.NoError(t, store.Set(creds.SourceToken, "source-token")) + require.NoError(t, store.Set(creds.TargetToken, "target-token")) + + root := NewRootCmd("test") + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs([]string{"config", "check"}) + err = root.Execute() + + require.Error(t, err) + assert.Empty(t, stdout.String()) + assert.Contains(t, stderr.String(), "[OK] Source network") + assert.Contains(t, stderr.String(), "[OK] Source service") + assert.Contains(t, stderr.String(), "[OK] Source authentication") + assert.Contains(t, stderr.String(), "[OK] Target network") + assert.Contains(t, stderr.String(), "[OK] Target service") + assert.Contains(t, stderr.String(), "[FAIL] Target authentication: HTTP 403 Forbidden") + }) + + t.Run("malformed config reports both endpoints", func(t *testing.T) { + configDir := t.TempDir() + t.Setenv("GH_ELM_CONFIG_DIR", configDir) + t.Setenv("GH_ELM_CREDENTIAL_STORE", "file") + require.NoError(t, os.WriteFile(filepath.Join(configDir, "config.json"), []byte("{"), 0o600)) + + assertBothEndpointsReported(t) + }) + + t.Run("invalid credential store reports both endpoints", func(t *testing.T) { + t.Setenv("GH_ELM_CONFIG_DIR", t.TempDir()) + t.Setenv("GH_ELM_CREDENTIAL_STORE", "invalid") + + assertBothEndpointsReported(t) + }) +} + func TestMigratorPATInput(t *testing.T) { t.Run("body flag", func(t *testing.T) { t.Setenv("SOURCE_PAT", "env-pat") diff --git a/internal/cmd/migration/migration.go b/internal/cmd/migration/migration.go index 1212ef0..0b90d1d 100644 --- a/internal/cmd/migration/migration.go +++ b/internal/cmd/migration/migration.go @@ -18,6 +18,7 @@ import ( "github.com/github/gh-elm/internal/config" "github.com/github/gh-elm/internal/elmapi" "github.com/github/gh-elm/internal/endpoints" + "github.com/github/gh-elm/internal/preflight" "github.com/github/gh-elm/internal/render" "github.com/github/gh-elm/internal/workflow" ) @@ -64,7 +65,7 @@ func NewCommand() *cobra.Command { // sourceClient resolves the source (GHES) endpoint (flags override env override // stored config) and returns a ready client plus the resolved base URL for error // messages. -func sourceClient(sourceURL, sourceToken string) (*elmapi.Client, string, error) { +func sourceClient(ctx context.Context, stderr io.Writer, sourceURL, sourceToken string) (*elmapi.Client, string, error) { resolver, err := endpoints.NewResolver() if err != nil { return nil, "", err @@ -73,11 +74,8 @@ func sourceClient(sourceURL, sourceToken string) (*elmapi.Client, string, error) if err != nil { return nil, "", err } - if ep.URL == "" { - return nil, "", fmt.Errorf("no source URL configured; run `gh elm config`, set %s, or pass --source-url", config.EnvSourceURL) - } - if ep.Token == "" { - return nil, "", fmt.Errorf("no source token configured; run `gh elm config`, set %s, or pass --source-token", config.EnvSourceToken) + if err := preflight.CheckEndpoint(ctx, stderr, "Source", ep.URL, ep.Token); err != nil { + return nil, ep.URL, err } return elmapi.NewClient(ep.URL, ep.Token), ep.URL, nil } @@ -154,11 +152,6 @@ func newCreateCmd() *cobra.Command { return errors.New("--json cannot be used with --watch") } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) - if err != nil { - return err - } - // WORKAROUND (API defect): the create endpoint requires a // target_api_endpoint, even though every other migration command here // only talks to the source (GHES) API and this CLI has no target-api @@ -173,6 +166,11 @@ func newCreateCmd() *cobra.Command { return fmt.Errorf("the create API requires a target endpoint; set %s (for example api.staffship-01.ghe.com) or run `gh elm config`", config.EnvTargetURL) } + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + if err != nil { + return err + } + req := elmapi.CreateMigrationRequest{ SourceOrganizationLogin: repositories.source.organization, SourceRepositoryName: repositories.source.repository, @@ -249,7 +247,7 @@ func newStartCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -288,7 +286,7 @@ func newStatusCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -337,7 +335,7 @@ func newTargetIDCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -390,7 +388,7 @@ func newListCmd() *cobra.Command { return err } } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -444,7 +442,7 @@ func newCancelCmd() *cobra.Command { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -489,7 +487,7 @@ func newCutoverCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -537,7 +535,7 @@ func newCutoverStatusCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -582,7 +580,7 @@ func newRevertCutoverCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -627,7 +625,7 @@ func newPauseCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } @@ -658,7 +656,7 @@ func newResumeCmd() *cobra.Command { if err != nil { return err } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } diff --git a/internal/cmd/migration/migration_test.go b/internal/cmd/migration/migration_test.go index 690b26a..6a4de3b 100644 --- a/internal/cmd/migration/migration_test.go +++ b/internal/cmd/migration/migration_test.go @@ -19,6 +19,9 @@ func TestCreate(t *testing.T) { var gotPath, gotMethod string var gotBody elmapiCreateBody srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } if r.Method == http.MethodGet { assert.Equal(t, elmapi.StatusCreated, r.URL.Query().Get("status")) assert.Equal(t, "100", r.URL.Query().Get("page_size")) @@ -155,6 +158,9 @@ func TestCreate(t *testing.T) { t.Run("checks every created migration page", func(t *testing.T) { var cursors []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } cursors = append(cursors, r.URL.Query().Get("after")) if r.URL.Query().Get("after") == "" { _, _ = w.Write([]byte(`{"migrations":[{ @@ -186,7 +192,10 @@ func TestCreate(t *testing.T) { }) t.Run("surfaces duplicate preflight failures", func(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } http.Error(w, "unavailable", http.StatusServiceUnavailable) })) defer srv.Close() @@ -243,6 +252,9 @@ func TestStart(t *testing.T) { t.Run("posts to the start endpoint", func(t *testing.T) { var gotPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } gotPath = r.URL.Path w.WriteHeader(http.StatusNoContent) })) @@ -266,22 +278,32 @@ func TestStatus(t *testing.T) { t.Run("prints human-readable status", func(t *testing.T) { const respBody = `{"migration":{"migration_id":"mig-1","status":"in_progress"},"target_state":null,"combined_state":null,"messages":[]}` srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } assert.True(t, strings.HasSuffix(r.URL.Path, "/enterprise/live-migrations/mig-1"), "path = %q", r.URL.Path) _, _ = w.Write([]byte(respBody)) })) defer srv.Close() - out := run(t, "status", "mig-1", + out, stderr, err := execStreams(t, "status", "mig-1", "--source-url", srv.URL, "--source-token", "tok") + require.NoError(t, err) for _, want := range []string{"Migration", "Migration ID", "mig-1", "In progress"} { assert.Contains(t, out, want) } + assert.Contains(t, stderr, "[OK] Source network") + assert.Contains(t, stderr, "[OK] Source service") + assert.Contains(t, stderr, "[OK] Source authentication") }) t.Run("--json preserves the raw status response", func(t *testing.T) { const respBody = `{"migration":{"migration_id":"mig-1"},"future_field":{"value":1}}` - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } _, _ = w.Write([]byte(respBody)) })) defer srv.Close() @@ -332,6 +354,9 @@ func TestList(t *testing.T) { const createdBody = `{"migrations":[{"migration_id":"created-id","status":"created"}],"total_count":1}` var statuses []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } status := r.URL.Query().Get("status") statuses = append(statuses, status) if status == "" { @@ -351,7 +376,10 @@ func TestList(t *testing.T) { t.Run("does not fall back when page size is explicit", func(t *testing.T) { var requests int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } requests++ _, _ = w.Write([]byte(`{"migrations":[],"total_count":0}`)) })) @@ -364,7 +392,10 @@ func TestList(t *testing.T) { t.Run("does not fall back when a cursor is explicit", func(t *testing.T) { var requests int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } requests++ _, _ = w.Write([]byte(`{"migrations":[],"total_count":0}`)) })) @@ -377,7 +408,10 @@ func TestList(t *testing.T) { t.Run("does not fall back when the default response is non-empty", func(t *testing.T) { var requests int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } requests++ _, _ = w.Write([]byte(`{"migrations":[{"migration_id":"active-id","status":"in_progress"}],"total_count":1}`)) })) @@ -391,7 +425,10 @@ func TestList(t *testing.T) { t.Run("does not fall back when migrations are returned despite a zero total count", func(t *testing.T) { var requests int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } requests++ _, _ = w.Write([]byte(`{"migrations":[{"migration_id":"active-id","status":"in_progress"}],"total_count":0}`)) })) @@ -431,6 +468,9 @@ func TestActions(t *testing.T) { t.Run(tc.name, func(t *testing.T) { var gotPath, gotMethod string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } gotPath = r.URL.Path gotMethod = r.Method w.WriteHeader(tc.respCode) @@ -457,6 +497,9 @@ func TestCancel(t *testing.T) { t.Run("accepts the kill alias", func(t *testing.T) { var gotPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } gotPath = r.URL.Path w.WriteHeader(http.StatusNoContent) })) @@ -470,6 +513,9 @@ func TestCancel(t *testing.T) { t.Run("accepts the migration ID flag", func(t *testing.T) { var gotPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } gotPath = r.URL.Path w.WriteHeader(http.StatusNoContent) })) @@ -529,6 +575,9 @@ func TestCutover(t *testing.T) { t.Run("sends force in the body", func(t *testing.T) { var gotForce bool srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } var body struct { Force bool `json:"force"` } @@ -547,6 +596,9 @@ func TestCutover(t *testing.T) { t.Run("accepts the old command name as an alias", func(t *testing.T) { var gotPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } gotPath = r.URL.Path w.WriteHeader(http.StatusNoContent) })) @@ -604,8 +656,32 @@ func TestRevertCutoverCompatibility(t *testing.T) { } func TestSourceErrorAnnotation(t *testing.T) { + t.Run("blocks status when authentication preflight fails", func(t *testing.T) { + var statusCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/user") { + http.Error(w, `{"message":"Bad credentials"}`, http.StatusUnauthorized) + return + } + statusCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + _, stderr, err := execStreams(t, "status", "m", "--source-url", srv.URL, "--source-token", "tok") + + require.Error(t, err) + assert.False(t, statusCalled) + assert.Contains(t, stderr, "[OK] Source network: reachable (HTTP 401 Unauthorized)") + assert.Contains(t, stderr, "[OK] Source service: responding (HTTP 401 Unauthorized)") + assert.Contains(t, stderr, "[FAIL] Source authentication: HTTP 401 Unauthorized; the configured token was rejected") + }) + t.Run("annotates an authentication failure", func(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write([]byte(`{"message":"Bad credentials"}`)) })) @@ -677,8 +753,14 @@ func TestSourceErrorAnnotation(t *testing.T) { }) t.Run("reports failed authentication behind an ELM 404", func(t *testing.T) { + userRequests := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.HasSuffix(r.URL.Path, "/user") { + userRequests++ + if userRequests == 1 { + w.WriteHeader(http.StatusOK) + return + } http.Error(w, `{"message":"Bad credentials"}`, http.StatusUnauthorized) return } @@ -693,6 +775,9 @@ func TestSourceErrorAnnotation(t *testing.T) { t.Run("preserves a missing migration error when ELM is available", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } if strings.HasSuffix(r.URL.Path, "/enterprise/live-migrations") { _, _ = w.Write([]byte(`{"migrations":[],"total_count":0,"next_cursor":""}`)) return @@ -772,6 +857,9 @@ func TestTargetID(t *testing.T) { t.Run("prints the target migration ID (human)", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } assert.True(t, strings.HasSuffix(r.URL.Path, "/enterprise/live-migrations/mig-1"), "path = %q", r.URL.Path) _, _ = w.Write([]byte(withTargetID)) })) @@ -802,7 +890,14 @@ func TestTargetID(t *testing.T) { // A mistyped / unknown migration ID returns 404 from GHES. The command // must surface that as an error rather than printing a target ID, so a // nonexistent migration is never mistaken for a successful lookup. - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSuccessfulPreflight(w, r) { + return + } + if strings.HasSuffix(r.URL.Path, "/enterprise/live-migrations") { + _, _ = w.Write([]byte(`{"migrations":[],"total_count":0,"next_cursor":""}`)) + return + } w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message":"Not Found"}`)) })) @@ -873,15 +968,28 @@ func runErr(t *testing.T, args ...string) error { } func exec(t *testing.T, args ...string) (string, error) { + stdout, _, err := execStreams(t, args...) + return stdout, err +} + +func execStreams(t *testing.T, args ...string) (stdoutText, stderrText string, err error) { t.Setenv("GH_ELM_CONFIG_DIR", t.TempDir()) t.Setenv("GH_ELM_CREDENTIAL_STORE", "file") cmd := NewCommand() - var buf bytes.Buffer - cmd.SetOut(&buf) - cmd.SetErr(&buf) + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) cmd.SetArgs(args) - err := cmd.Execute() - return buf.String(), err + err = cmd.Execute() + return stdout.String(), stderr.String(), err +} + +func serveSuccessfulPreflight(w http.ResponseWriter, r *http.Request) bool { + if strings.HasSuffix(r.URL.Path, "/meta") || strings.HasSuffix(r.URL.Path, "/user") { + w.WriteHeader(http.StatusOK) + return true + } + return false } diff --git a/internal/cmd/migration/watch.go b/internal/cmd/migration/watch.go index 06606ba..97f0876 100644 --- a/internal/cmd/migration/watch.go +++ b/internal/cmd/migration/watch.go @@ -162,7 +162,7 @@ func newWatchCmd() *cobra.Command { return fmt.Errorf("interval must be positive, got %s", interval) } - client, srcURL, err := sourceClient(*sourceURLFlag(cmd), *sourceTokenFlag(cmd)) + client, srcURL, err := sourceClient(cmd.Context(), cmd.ErrOrStderr(), *sourceURLFlag(cmd), *sourceTokenFlag(cmd)) if err != nil { return err } diff --git a/internal/cmd/root_test.go b/internal/cmd/root_test.go index d21ffd4..7f2558c 100644 --- a/internal/cmd/root_test.go +++ b/internal/cmd/root_test.go @@ -123,6 +123,7 @@ func TestRootHelp(t *testing.T) { assert.Contains(t, help, "\nUSAGE\n gh elm [flags]\n") assert.Contains(t, help, "\nCOMMANDS\n") assert.Contains(t, help, " config:") + assert.Contains(t, help, " config check:") assert.Contains(t, help, " config show:") assert.Contains(t, help, " config reset:") assert.Contains(t, help, " help: Help about any command") diff --git a/internal/elmapi/elmapi.go b/internal/elmapi/elmapi.go index 879a803..67bad20 100644 --- a/internal/elmapi/elmapi.go +++ b/internal/elmapi/elmapi.go @@ -92,7 +92,14 @@ func NewClient(baseURL, token string, opts ...Option) *Client { // CheckAuthentication verifies that the configured token can access the // authenticated-user endpoint. func (c *Client) CheckAuthentication(ctx context.Context) error { - if err := c.get(ctx, "/user", nil, nil); err != nil { + probeClient := *c + httpClient := *c.httpClient + httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + probeClient.httpClient = &httpClient + + if err := probeClient.get(ctx, "/user", nil, nil); err != nil { return fmt.Errorf("checking authentication: %w", err) } return nil diff --git a/internal/elmapi/elmapi_test.go b/internal/elmapi/elmapi_test.go index d40c892..2b309cd 100644 --- a/internal/elmapi/elmapi_test.go +++ b/internal/elmapi/elmapi_test.go @@ -48,3 +48,21 @@ func TestHTTPErrorFallsBackToRawBody(t *testing.T) { assert.Empty(t, httpErr.DocumentationURL) assert.Empty(t, httpErr.CorrelationID) } + +func TestCheckAuthenticationDoesNotFollowRedirects(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/user" { + http.Redirect(w, r, "/login", http.StatusFound) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + err := NewClient(srv.URL, "tok").CheckAuthentication(t.Context()) + require.Error(t, err) + + var httpErr *HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusFound, httpErr.StatusCode) +} diff --git a/internal/preflight/preflight.go b/internal/preflight/preflight.go new file mode 100644 index 0000000..5170599 --- /dev/null +++ b/internal/preflight/preflight.go @@ -0,0 +1,84 @@ +// Package preflight checks API connectivity and authentication before commands +// perform ELM operations. +package preflight + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "strings" + + "github.com/github/gh-elm/internal/elmapi" +) + +// CheckEndpoint reports network reachability, API service health, and +// authentication independently. It runs every applicable check even after a +// failure so the output is useful for diagnosing configuration problems. +func CheckEndpoint(ctx context.Context, stderr io.Writer, name, baseURL, token string) error { + name = strings.TrimSpace(name) + if baseURL == "" { + fmt.Fprintf(stderr, "[FAIL] %s network: URL is not configured\n", name) + fmt.Fprintf(stderr, "[FAIL] %s service: not checked because the URL is not configured\n", name) + fmt.Fprintf(stderr, "[FAIL] %s authentication: not checked because the URL is not configured\n", name) + return fmt.Errorf("%s preflight failed: URL is not configured; run `gh elm config`", strings.ToLower(name)) + } + + userEndpoint := strings.TrimRight(baseURL, "/") + "/user" + if request, err := http.NewRequestWithContext(ctx, http.MethodGet, userEndpoint, http.NoBody); err == nil { + request.URL.User = nil + userEndpoint = request.URL.String() + } + client := elmapi.NewClient(baseURL, token) + authenticationErr := client.CheckAuthentication(ctx) + var httpErr *elmapi.HTTPError + switch { + case authenticationErr == nil: + fmt.Fprintf(stderr, "[OK] %s network: reachable (HTTP 200 OK)\n", name) + fmt.Fprintf(stderr, "[OK] %s service: responding (HTTP 200 OK)\n", name) + case errors.As(authenticationErr, &httpErr): + status := fmt.Sprintf("HTTP %d %s", httpErr.StatusCode, http.StatusText(httpErr.StatusCode)) + fmt.Fprintf(stderr, "[OK] %s network: reachable (%s)\n", name, status) + if httpErr.StatusCode >= http.StatusInternalServerError { + fmt.Fprintf(stderr, "[FAIL] %s service: unhealthy (%s)\n", name, status) + } else { + fmt.Fprintf(stderr, "[OK] %s service: responding (%s)\n", name, status) + } + default: + fmt.Fprintf(stderr, "[FAIL] %s network: %v\n", name, authenticationErr) + fmt.Fprintf(stderr, "[FAIL] %s service: not checked because the network probe failed\n", name) + } + + switch { + case token == "": + fmt.Fprintf(stderr, "[FAIL] %s authentication: token is not configured\n", name) + case authenticationErr != nil: + fmt.Fprintf(stderr, "[FAIL] %s authentication: %s\n", name, authenticationFailure(authenticationErr, userEndpoint)) + default: + fmt.Fprintf(stderr, "[OK] %s authentication: credentials accepted by %s\n", name, userEndpoint) + return nil + } + + return fmt.Errorf("%s preflight failed; see checks above", strings.ToLower(name)) +} + +func authenticationFailure(err error, userEndpoint string) string { + var httpErr *elmapi.HTTPError + if !errors.As(err, &httpErr) { + return fmt.Sprintf("could not verify credentials: %v", err) + } + + status := fmt.Sprintf("HTTP %d %s", httpErr.StatusCode, http.StatusText(httpErr.StatusCode)) + switch httpErr.StatusCode { + case http.StatusUnauthorized: + return status + "; the configured token was rejected or has expired" + case http.StatusForbidden: + return status + "; the configured token lacks access to " + userEndpoint + default: + if httpErr.StatusCode >= http.StatusInternalServerError { + return status + "; credentials could not be verified because the API service is unhealthy" + } + return fmt.Sprintf("%s: %s", status, httpErr.Message) + } +} diff --git a/internal/preflight/preflight_test.go b/internal/preflight/preflight_test.go new file mode 100644 index 0000000..ce3794a --- /dev/null +++ b/internal/preflight/preflight_test.go @@ -0,0 +1,42 @@ +package preflight + +import ( + "bytes" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheckEndpoint(t *testing.T) { + t.Run("reports a server failure separately from network reachability", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "unavailable", http.StatusServiceUnavailable) + })) + defer server.Close() + + var stderr bytes.Buffer + err := CheckEndpoint(t.Context(), &stderr, "Source", server.URL, "token") + + require.Error(t, err) + assert.Contains(t, stderr.String(), "[OK] Source network: reachable (HTTP 503 Service Unavailable)") + assert.Contains(t, stderr.String(), "[FAIL] Source service: unhealthy (HTTP 503 Service Unavailable)") + assert.Contains(t, stderr.String(), "[FAIL] Source authentication: HTTP 503 Service Unavailable; credentials could not be verified") + }) + + t.Run("reports a transport failure separately from authentication", func(t *testing.T) { + server := httptest.NewServer(http.NotFoundHandler()) + serverURL := server.URL + server.Close() + + var stderr bytes.Buffer + err := CheckEndpoint(t.Context(), &stderr, "Source", serverURL, "token") + + require.Error(t, err) + assert.Contains(t, stderr.String(), "[FAIL] Source network:") + assert.Contains(t, stderr.String(), "[FAIL] Source service: not checked because the network probe failed") + assert.Contains(t, stderr.String(), "[FAIL] Source authentication: could not verify credentials") + }) +}