Improved request errors (#286)

* worked on wrapping errors in requests.go, added defer statements

* removed .idea (generated by goland)

* added another require.NoError

* Update pkg/requests/requests.go

Co-Authored-By: Dan Bond <pm@danbond.io>

* fixed out-of-order imports

* changelog entry added

* swapped error definitions to use fmt.Errorf rather than Wrap()

* formatting changes, added new defers to requests_test.go

* suppot for go1.12 pipeline removed from travis pipeline, .idea/ added to gitignore

* Reorder changelog entry

.gitignore

@@ -16,6 +16,7 @@ release
 # Folders
 _obj
 _test
+.idea/
 
 # Architecture specific extensions/prefixes
 *.[568vq]

.travis.yml

@@ -1,6 +1,5 @@
 language: go
 go:
-  - 1.12.x
   - 1.13.x
 install:
   # Fetch dependencies

CHANGELOG.md

@@ -8,6 +8,7 @@
 - [#275](https://github.com/pusher/oauth2_proxy/pull/275) docker: build from debian buster (@syscll)
 - [#258](https://github.com/pusher/oauth2_proxy/pull/258) Add IDToken for Azure provider
   - This PR adds the IDToken into the session for the Azure provider allowing requests to a backend to be identified as a specific user. As a consequence, if you are using a cookie to store the session the cookie will now exceed the 4kb size limit and be split into multiple cookies. This can cause problems when using nginx as a proxy, resulting in no cookie being passed at all. Either increase the proxy_buffer_size in nginx or implement the redis session storage (see https://pusher.github.io/oauth2_proxy/configuration#redis-storage)
+- [#286](https://github.com/pusher/oauth2_proxy/pull/286) Requests.go updated with useful error messages (@biotom)
 
 # v4.0.0
 

pkg/requests/requests.go

@@ -18,17 +18,23 @@ func Request(req *http.Request) (*simplejson.Json, error) {
 		return nil, err
 	}
 	body, err := ioutil.ReadAll(resp.Body)
-	resp.Body.Close()
-	logger.Printf("%d %s %s %s", resp.StatusCode, req.Method, req.URL, body)
-	if err != nil {
-		return nil, err
+	if body != nil {
+		defer resp.Body.Close()
 	}
+
+	logger.Printf("%d %s %s %s", resp.StatusCode, req.Method, req.URL, body)
+
+	if err != nil {
+		return nil, fmt.Errorf("problem reading http request body: %w", err)
+	}
+
 	if resp.StatusCode != 200 {
 		return nil, fmt.Errorf("got %d %s", resp.StatusCode, body)
 	}
+
 	data, err := simplejson.NewJson(body)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("error unmarshalling json: %w", err)
 	}
 	return data, nil
 }
@@ -41,10 +47,13 @@ func RequestJSON(req *http.Request, v interface{}) error {
 		return err
 	}
 	body, err := ioutil.ReadAll(resp.Body)
-	resp.Body.Close()
+	if body != nil {
+		defer resp.Body.Close()
+	}
+
 	logger.Printf("%d %s %s %s", resp.StatusCode, req.Method, req.URL, body)
 	if err != nil {
-		return err
+		return fmt.Errorf("error reading body from http response: %w", err)
 	}
 	if resp.StatusCode != 200 {
 		return fmt.Errorf("got %d %s", resp.StatusCode, body)
@@ -56,7 +65,7 @@ func RequestJSON(req *http.Request, v interface{}) error {
 func RequestUnparsedResponse(url string, header http.Header) (resp *http.Response, err error) {
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("error performing get request: %w", err)
 	}
 	req.Header = header
 

pkg/requests/requests_test.go

@@ -8,20 +8,21 @@ import (
 	"testing"
 
 	"github.com/bitly/go-simplejson"
-
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func testBackend(responseCode int, payload string) *httptest.Server {
+func testBackend(t *testing.T, responseCode int, payload string) *httptest.Server {
 	return httptest.NewServer(http.HandlerFunc(
 		func(w http.ResponseWriter, r *http.Request) {
 			w.WriteHeader(responseCode)
-			w.Write([]byte(payload))
+			_, err := w.Write([]byte(payload))
+			require.NoError(t, err)
 		}))
 }
 
 func TestRequest(t *testing.T) {
-	backend := testBackend(200, "{\"foo\": \"bar\"}")
+	backend := testBackend(t, 200, "{\"foo\": \"bar\"}")
 	defer backend.Close()
 
 	req, _ := http.NewRequest("GET", backend.URL, nil)
@@ -35,7 +36,7 @@ func TestRequest(t *testing.T) {
 func TestRequestFailure(t *testing.T) {
 	// Create a backend to generate a test URL, then close it to cause a
 	// connection error.
-	backend := testBackend(200, "{\"foo\": \"bar\"}")
+	backend := testBackend(t, 200, "{\"foo\": \"bar\"}")
 	backend.Close()
 
 	req, err := http.NewRequest("GET", backend.URL, nil)
@@ -49,7 +50,7 @@ func TestRequestFailure(t *testing.T) {
 }
 
 func TestHttpErrorCode(t *testing.T) {
-	backend := testBackend(404, "{\"foo\": \"bar\"}")
+	backend := testBackend(t, 404, "{\"foo\": \"bar\"}")
 	defer backend.Close()
 
 	req, err := http.NewRequest("GET", backend.URL, nil)
@@ -60,7 +61,7 @@ func TestHttpErrorCode(t *testing.T) {
 }
 
 func TestJsonParsingError(t *testing.T) {
-	backend := testBackend(200, "not well-formed JSON")
+	backend := testBackend(t, 200, "not well-formed JSON")
 	defer backend.Close()
 
 	req, err := http.NewRequest("GET", backend.URL, nil)
@@ -77,7 +78,8 @@ func TestRequestUnparsedResponseUsingAccessTokenParameter(t *testing.T) {
 			token := r.FormValue("access_token")
 			if r.URL.Path == "/" && token == "my_token" {
 				w.WriteHeader(200)
-				w.Write([]byte("some payload"))
+				_, err := w.Write([]byte("some payload"))
+				require.NoError(t, err)
 			} else {
 				w.WriteHeader(403)
 			}
@@ -86,16 +88,17 @@ func TestRequestUnparsedResponseUsingAccessTokenParameter(t *testing.T) {
 
 	response, err := RequestUnparsedResponse(
 		backend.URL+"?access_token=my_token", nil)
+	defer response.Body.Close()
+
 	assert.Equal(t, nil, err)
 	assert.Equal(t, 200, response.StatusCode)
 	body, err := ioutil.ReadAll(response.Body)
 	assert.Equal(t, nil, err)
-	response.Body.Close()
 	assert.Equal(t, "some payload", string(body))
 }
 
 func TestRequestUnparsedResponseUsingAccessTokenParameterFailedResponse(t *testing.T) {
-	backend := testBackend(200, "some payload")
+	backend := testBackend(t, 200, "some payload")
 	// Close the backend now to force a request failure.
 	backend.Close()
 
@@ -110,7 +113,8 @@ func TestRequestUnparsedResponseUsingHeaders(t *testing.T) {
 		func(w http.ResponseWriter, r *http.Request) {
 			if r.URL.Path == "/" && r.Header["Auth"][0] == "my_token" {
 				w.WriteHeader(200)
-				w.Write([]byte("some payload"))
+				_, err := w.Write([]byte("some payload"))
+				require.NoError(t, err)
 			} else {
 				w.WriteHeader(403)
 			}
@@ -120,10 +124,12 @@ func TestRequestUnparsedResponseUsingHeaders(t *testing.T) {
 	headers := make(http.Header)
 	headers.Set("Auth", "my_token")
 	response, err := RequestUnparsedResponse(backend.URL, headers)
+	defer response.Body.Close()
+
 	assert.Equal(t, nil, err)
 	assert.Equal(t, 200, response.StatusCode)
 	body, err := ioutil.ReadAll(response.Body)
 	assert.Equal(t, nil, err)
-	response.Body.Close()
+
 	assert.Equal(t, "some payload", string(body))
 }