removed API token, handling ReadAll error

Author: ubettigole

submitqueue/extension/changeprovider/phabricator/conduit.go

@@ -33,14 +33,11 @@ type fileChange struct {
 }
 
 // buildQueryDiffsRequest builds query parameters for a differential.querydiffs call.
-func buildQueryDiffsRequest(diffIDs []int, apiToken string) url.Values {
+func buildQueryDiffsRequest(diffIDs []int) url.Values {
 	form := url.Values{}
 	for _, id := range diffIDs {
 		form.Add("ids[]", strconv.Itoa(id))
 	}
-	if apiToken != "" {
-		form.Set("api.token", apiToken)
-	}
 	return form
 }
 
@@ -66,7 +63,10 @@ func doConduitRequest(ctx context.Context, httpClient *http.Client, form url.Val
 // and extracts the diffResults for the requested diff IDs.
 func parseConduitResponse(resp *http.Response, diffIDs []int) (map[int]*diffResult, error) {
 	if resp.StatusCode != http.StatusOK {
-		body, _ := io.ReadAll(resp.Body)
+		body, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return nil, fmt.Errorf("Conduit API returned status %d and failed to read body: %w", resp.StatusCode, err)
+		}
 		return nil, fmt.Errorf("Conduit API returned status %d: %s", resp.StatusCode, string(body))
 	}
 

submitqueue/extension/changeprovider/phabricator/conduit_test.go

@@ -2,6 +2,7 @@ package phabricator
 
 import (
 	"encoding/json"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -14,10 +15,9 @@ import (
 
 func TestBuildQueryDiffsRequest(t *testing.T) {
 	testCases := []struct {
-		name     string
-		diffIDs  []int
-		apiToken string
-		wantAll  []string
+		name    string
+		diffIDs []int
+		wantAll []string
 	}{
 		{
 			name:    "single ID",
@@ -29,17 +29,11 @@ func TestBuildQueryDiffsRequest(t *testing.T) {
 			diffIDs: []int{42, 43, 44},
 			wantAll: []string{"ids%5B%5D=42", "ids%5B%5D=43", "ids%5B%5D=44"},
 		},
-		{
-			name:     "includes api.token when set",
-			diffIDs:  []int{42},
-			apiToken: "my-secret",
-			wantAll:  []string{"ids%5B%5D=42", "api.token=my-secret"},
-		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			form := buildQueryDiffsRequest(tc.diffIDs, tc.apiToken)
+			form := buildQueryDiffsRequest(tc.diffIDs)
 			encoded := form.Encode()
 			for _, want := range tc.wantAll {
 				assert.Contains(t, encoded, want)
@@ -60,7 +54,7 @@ func TestDoConduitRequest(t *testing.T) {
 	defer server.Close()
 
 	client := &http.Client{Transport: &testTransport{baseURL: server.URL}}
-	form := buildQueryDiffsRequest([]int{100, 200}, "")
+	form := buildQueryDiffsRequest([]int{100, 200})
 
 	resp, err := doConduitRequest(t.Context(), client, form)
 	require.NoError(t, err)
@@ -73,7 +67,7 @@ func TestDoConduitRequest(t *testing.T) {
 
 func TestDoConduitRequest_ConnectionError(t *testing.T) {
 	client := &http.Client{Transport: &testTransport{baseURL: "http://127.0.0.1:0"}}
-	form := buildQueryDiffsRequest([]int{1}, "")
+	form := buildQueryDiffsRequest([]int{1})
 
 	_, err := doConduitRequest(t.Context(), client, form)
 
@@ -104,6 +98,15 @@ func TestParseConduitResponse(t *testing.T) {
 			diffIDs: []int{100},
 			wantErr: "Conduit API returned status 500",
 		},
+		{
+			name: "HTTP error with unreadable body",
+			resp: &http.Response{
+				StatusCode: http.StatusBadGateway,
+				Body:       io.NopCloser(&errReader{}),
+			},
+			diffIDs: []int{100},
+			wantErr: "failed to read body",
+		},
 		{
 			name:    "conduit error",
 			resp:    newConduitErrorHTTPResponse(t, "ERR-CONDUIT-CORE", "Invalid diff ID"),
@@ -200,3 +203,9 @@ func (t *testTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	req.URL.Host = t.baseURL[len("http://"):]
 	return http.DefaultTransport.RoundTrip(req)
 }
+
+type errReader struct{}
+
+func (e *errReader) Read([]byte) (int, error) {
+	return 0, fmt.Errorf("simulated read error")
+}

submitqueue/extension/changeprovider/phabricator/provider.go

@@ -22,10 +22,8 @@ const queryDiffsBatchSize = 10
 type Params struct {
 	// HTTPClient is a pre-configured HTTP client. The caller is responsible for
 	// configuring the base URL (e.g. via httpclient.NewClient) and authentication
-	// (e.g. via oauth2.Transport for Bearer tokens) via transport layers.
+	// (e.g. via a RoundTripper that injects credentials) via transport layers.
 	HTTPClient *http.Client
-	// APIToken is the Conduit API token, sent as the api.token form parameter.
-	APIToken string
 	// Logger is the structured logger.
 	Logger *zap.SugaredLogger
 	// MetricsScope is the metrics scope for instrumentation.
@@ -34,7 +32,6 @@ type Params struct {
 
 type provider struct {
 	httpClient   *http.Client
-	apiToken     string
 	logger       *zap.SugaredLogger
 	metricsScope tally.Scope
 }
@@ -43,7 +40,6 @@ type provider struct {
 func NewProvider(params Params) changeprovider.ChangeProvider {
 	return &provider{
 		httpClient:   params.HTTPClient,
-		apiToken:     params.APIToken,
 		logger:       params.Logger.Named("phabricator_changeprovider"),
 		metricsScope: params.MetricsScope.SubScope("phabricator_changeprovider"),
 	}
@@ -134,7 +130,7 @@ func (p *provider) fetchAllDiffs(ctx context.Context, changeIDs []entityphab.Cha
 
 // fetchDiffBatch fetches a single batch of diffs from Phabricator.
 func (p *provider) fetchDiffBatch(ctx context.Context, diffIDs []int) (map[int]*diffResult, error) {
-	form := buildQueryDiffsRequest(diffIDs, p.apiToken)
+	form := buildQueryDiffsRequest(diffIDs)
 
 	resp, err := doConduitRequest(ctx, p.httpClient, form)
 	if err != nil {