From 6aec75ed439529d2fd5249794a1edd493f156543 Mon Sep 17 00:00:00 2001 From: Sergiu Ghitea Date: Sat, 27 Aug 2022 16:25:58 +0200 Subject: [PATCH] Enhance reliability of how we make requests and parse errors --- management/anomaly.go | 11 ++--- management/branding.go | 17 +------- management/guardian.go | 12 +++--- management/job.go | 27 +------------ management/management_error.go | 24 ++++++++--- management/management_error_test.go | 62 +++++++++++++++++++++++++++++ management/management_request.go | 27 ++++++++----- management/management_test.go | 2 +- management/user.go | 24 ++++++----- 9 files changed, 127 insertions(+), 79 deletions(-) create mode 100644 management/management_error_test.go diff --git a/management/anomaly.go b/management/anomaly.go index d75d9484..a088be33 100644 --- a/management/anomaly.go +++ b/management/anomaly.go @@ -18,27 +18,28 @@ func newAnomalyManager(m *Management) *AnomalyManager { // // See: https://auth0.com/docs/api/management/v2#!/Anomaly/get_ips_by_id func (m *AnomalyManager) CheckIP(ip string, opts ...RequestOption) (isBlocked bool, err error) { - req, err := m.NewRequest("GET", m.URI("anomaly", "blocks", "ips", ip), nil, opts...) + request, err := m.NewRequest("GET", m.URI("anomaly", "blocks", "ips", ip), nil, opts...) if err != nil { return false, err } - res, err := m.Do(req) + response, err := m.Do(request) if err != nil { return false, err } + defer response.Body.Close() // 200: IP address specified is currently blocked. - if res.StatusCode == http.StatusOK { + if response.StatusCode == http.StatusOK { return true, nil } // 404: IP address specified is not currently blocked. - if res.StatusCode == http.StatusNotFound { + if response.StatusCode == http.StatusNotFound { return false, nil } - return false, newError(res.Body) + return false, newError(response) } // UnblockIP unblocks an IP address currently blocked by the multiple diff --git a/management/branding.go b/management/branding.go index d1433f96..d16a6637 100644 --- a/management/branding.go +++ b/management/branding.go @@ -3,7 +3,6 @@ package management import ( "encoding/json" "fmt" - "net/http" ) // Branding is used to customize the look and feel of Auth0 to align @@ -168,21 +167,7 @@ func (m *BrandingManager) UniversalLogin(opts ...RequestOption) (ul *BrandingUni // // See: https://auth0.com/docs/api/management/v2#!/Branding/put_universal_login func (m *BrandingManager) SetUniversalLogin(ul *BrandingUniversalLogin, opts ...RequestOption) (err error) { - req, err := m.NewRequest("PUT", m.URI("branding", "templates", "universal-login"), ul.Body, opts...) - if err != nil { - return err - } - - res, err := m.Do(req) - if err != nil { - return err - } - - if res.StatusCode >= http.StatusBadRequest { - return newError(res.Body) - } - - return nil + return m.Request("PUT", m.URI("branding", "templates", "universal-login"), ul.Body, opts...) } // DeleteUniversalLogin deletes the template for the New Universal Login Experience. diff --git a/management/guardian.go b/management/guardian.go index a640936e..3727a747 100644 --- a/management/guardian.go +++ b/management/guardian.go @@ -146,23 +146,23 @@ type EnrollmentTicket struct { // // See: https://auth0.com/docs/api/management/v2#!/Guardian/post_ticket func (m *EnrollmentManager) CreateTicket(t *CreateEnrollmentTicket, opts ...RequestOption) (EnrollmentTicket, error) { - req, err := m.NewRequest("POST", m.URI("guardian", "enrollments", "ticket"), t, opts...) + request, err := m.NewRequest("POST", m.URI("guardian", "enrollments", "ticket"), t, opts...) if err != nil { return EnrollmentTicket{}, err } - res, err := m.Do(req) + response, err := m.Do(request) if err != nil { return EnrollmentTicket{}, err } - defer res.Body.Close() + defer response.Body.Close() - if res.StatusCode != http.StatusOK { - return EnrollmentTicket{}, newError(res.Body) + if response.StatusCode != http.StatusOK { + return EnrollmentTicket{}, newError(response) } var out EnrollmentTicket - err = json.NewDecoder(res.Body).Decode(&out) + err = json.NewDecoder(response.Body).Decode(&out) return out, err } diff --git a/management/job.go b/management/job.go index 89b9df89..ba16284c 100644 --- a/management/job.go +++ b/management/job.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/json" "mime/multipart" - "net/http" "net/textproto" "strconv" "time" @@ -126,29 +125,7 @@ func (m *JobManager) ImportUsers(j *Job, opts ...RequestOption) error { } mp.Close() - req, err := http.NewRequest("POST", m.URI("jobs", "users-imports"), &payload) - if err != nil { - return err - } - req.Header.Add("Content-Type", mp.FormDataContentType()) - - for _, option := range opts { - option.apply(req) - } - - res, err := m.Do(req) - if err != nil { - return err - } - - if res.StatusCode < http.StatusOK || res.StatusCode >= http.StatusBadRequest { - return newError(res.Body) - } - - if res.StatusCode != http.StatusNoContent { - defer res.Body.Close() - return json.NewDecoder(res.Body).Decode(j) - } + opts = append(opts, Header("Content-Type", mp.FormDataContentType())) - return nil + return m.Request("POST", m.URI("jobs", "users-imports"), &payload, opts...) } diff --git a/management/management_error.go b/management/management_error.go index f626ff9a..0b1a40c9 100644 --- a/management/management_error.go +++ b/management/management_error.go @@ -3,7 +3,7 @@ package management import ( "encoding/json" "fmt" - "io" + "net/http" ) // Error is an interface describing any error which @@ -21,13 +21,25 @@ type managementError struct { Message string `json:"message"` } -func newError(r io.Reader) error { - m := &managementError{} - if err := json.NewDecoder(r).Decode(m); err != nil { - return err +func newError(response *http.Response) error { + apiError := &managementError{} + + if err := json.NewDecoder(response.Body).Decode(apiError); err != nil { + return &managementError{ + StatusCode: response.StatusCode, + Err: http.StatusText(response.StatusCode), + Message: fmt.Errorf("failed to decode json error response payload: %w", err).Error(), + } + } + + // This can happen in case the error message structure changes. + // If that happens we still want to display the correct code. + if apiError.Status() == 0 { + apiError.StatusCode = response.StatusCode + apiError.Err = http.StatusText(response.StatusCode) } - return m + return apiError } // Error formats the error into a string representation. diff --git a/management/management_error_test.go b/management/management_error_test.go new file mode 100644 index 00000000..e80e26d9 --- /dev/null +++ b/management/management_error_test.go @@ -0,0 +1,62 @@ +package management + +import ( + "io" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewError(t *testing.T) { + var testCases = []struct { + name string + givenResponse http.Response + expectedError managementError + }{ + { + name: "it fails to decode if body is not a json", + givenResponse: http.Response{ + StatusCode: http.StatusForbidden, + Body: io.NopCloser(strings.NewReader("Hello, I'm not a JSON.")), + }, + expectedError: managementError{ + StatusCode: 403, + Err: "Forbidden", + Message: "failed to decode json error response payload: invalid character 'H' looking for beginning of value", + }, + }, + { + name: "it correctly decodes the error response payload", + givenResponse: http.Response{ + StatusCode: http.StatusBadRequest, + Body: io.NopCloser(strings.NewReader(`{"statusCode":400,"error":"Bad Request","message":"One of 'client_id' or 'name' is required."}`)), + }, + expectedError: managementError{ + StatusCode: 400, + Err: "Bad Request", + Message: "One of 'client_id' or 'name' is required.", + }, + }, + { + name: "it will still post the correct status code if the body doesn't have the correct structure", + givenResponse: http.Response{ + StatusCode: http.StatusInternalServerError, + Body: io.NopCloser(strings.NewReader(`{"errorMessage":"wrongStruct"}`)), + }, + expectedError: managementError{ + StatusCode: 500, + Err: "Internal Server Error", + Message: "", + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + actualError := newError(&testCase.givenResponse) + assert.Equal(t, &testCase.expectedError, actualError) + }) + } +} diff --git a/management/management_request.go b/management/management_request.go index 5370bb87..f106f721 100644 --- a/management/management_request.go +++ b/management/management_request.go @@ -84,27 +84,32 @@ func (m *Management) Do(req *http.Request) (*http.Response, error) { } // Request combines NewRequest and Do, while also handling decoding of response payload. -func (m *Management) Request(method, uri string, v interface{}, options ...RequestOption) error { - request, err := m.NewRequest(method, uri, v, options...) +func (m *Management) Request(method, uri string, payload interface{}, options ...RequestOption) error { + request, err := m.NewRequest(method, uri, payload, options...) if err != nil { - return err + return fmt.Errorf("failed to create a new request: %w", err) } response, err := m.Do(request) if err != nil { - return fmt.Errorf("request failed: %w", err) + return fmt.Errorf("failed to send the request: %w", err) } + defer response.Body.Close() - if response.StatusCode < http.StatusOK || response.StatusCode >= http.StatusBadRequest { - return newError(response.Body) + // If the response contains a client or a server error then return the error. + if response.StatusCode >= http.StatusBadRequest { + return newError(response) } - if response.StatusCode != http.StatusNoContent && response.StatusCode != http.StatusAccepted { - if err := json.NewDecoder(response.Body).Decode(v); err != nil { - return fmt.Errorf("decoding response payload failed: %w", err) - } + responseBody, err := io.ReadAll(response.Body) + if err != nil { + return fmt.Errorf("failed to read the response body: %w", err) + } - return response.Body.Close() + if len(responseBody) > 0 && string(responseBody) != "{}" { + if err = json.Unmarshal(responseBody, &payload); err != nil { + return fmt.Errorf("failed to unmarshal response payload: %w", err) + } } return nil diff --git a/management/management_test.go b/management/management_test.go index ad64e62d..79120690 100644 --- a/management/management_test.go +++ b/management/management_test.go @@ -129,7 +129,7 @@ func TestOptionParameter(t *testing.T) { assert.Equal(t, "xyz", bar) } -func TestOptionDefauls(t *testing.T) { +func TestOptionDefaults(t *testing.T) { r, _ := http.NewRequest("GET", "/", nil) applyListDefaults([]RequestOption{ diff --git a/management/user.go b/management/user.go index 6c739577..bf6caa22 100644 --- a/management/user.go +++ b/management/user.go @@ -2,6 +2,8 @@ package management import ( "encoding/json" + "fmt" + "io" "net/http" "reflect" "strconv" @@ -530,26 +532,30 @@ func (m *UserManager) InvalidateRememberBrowser(id string, opts ...RequestOption // // See: https://auth0.com/docs/api/management/v2#!/Users/post_identities func (m *UserManager) Link(id string, il *UserIdentityLink, opts ...RequestOption) (uIDs []UserIdentity, err error) { - req, err := m.NewRequest("POST", m.URI("users", id, "identities"), il, opts...) + request, err := m.NewRequest("POST", m.URI("users", id, "identities"), il, opts...) if err != nil { return uIDs, err } - res, err := m.Do(req) + response, err := m.Do(request) if err != nil { return uIDs, err } + defer response.Body.Close() - if res.StatusCode < http.StatusOK || res.StatusCode >= http.StatusBadRequest { - return uIDs, newError(res.Body) + if response.StatusCode >= http.StatusBadRequest { + return uIDs, newError(response) } - if res.StatusCode != http.StatusNoContent && res.StatusCode != http.StatusAccepted { - err := json.NewDecoder(res.Body).Decode(&uIDs) - if err != nil { - return uIDs, err + responseBody, err := io.ReadAll(response.Body) + if err != nil { + return uIDs, fmt.Errorf("failed to read the response body: %w", err) + } + + if len(responseBody) > 0 { + if err = json.Unmarshal(responseBody, &uIDs); err != nil { + return uIDs, fmt.Errorf("failed to unmarshal response payload: %w", err) } - return uIDs, res.Body.Close() } return uIDs, nil