From d89fc2c8a8f66987f04000df32520d6d880b3ee9 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Wed, 12 Aug 2020 14:37:05 -0500 Subject: [PATCH] address review comments --- v4/core/authenticator.go | 2 +- v4/core/base_service.go | 10 ++++++-- v4/core/base_service_test.go | 4 --- v4/core/basic_authenticator.go | 2 +- v4/core/bearer_token_authenticator.go | 2 +- v4/core/config_utils_test.go | 5 ++-- v4/core/constants.go | 1 + v4/core/cp4d_authenticator.go | 2 +- v4/core/cp4d_authenticator_test.go | 33 +++++++++++++++++++++++++ v4/core/detailed_response.go | 2 +- v4/core/iam_authenticator.go | 2 +- v4/core/iam_authenticator_test.go | 33 +++++++++++++++++++++++++ v4/core/marshal_nulls_test.go | 11 ++++----- v4/core/noauth_authenticator.go | 2 +- v4/core/unmarshal_v2.go | 2 +- v4/core/unmarshal_v2_primitives_test.go | 8 +++--- 16 files changed, 94 insertions(+), 27 deletions(-) diff --git a/v4/core/authenticator.go b/v4/core/authenticator.go index 6a4361c..74b8548 100644 --- a/v4/core/authenticator.go +++ b/v4/core/authenticator.go @@ -21,7 +21,7 @@ import ( // Authenticator describes the set of methods implemented by each authenticator. type Authenticator interface { AuthenticationType() string - Authenticate(*http.Request) *AuthenticationError + Authenticate(*http.Request) error Validate() error } diff --git a/v4/core/base_service.go b/v4/core/base_service.go index 8364b0c..9c8ca83 100644 --- a/v4/core/base_service.go +++ b/v4/core/base_service.go @@ -221,8 +221,14 @@ func (service *BaseService) Request(req *http.Request, result interface{}) (deta authError := service.Options.Authenticator.Authenticate(req) if authError != nil { - err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, authError.Error()) - return authError.Response, err + castErr, ok := authError.(*AuthenticationError) + if !ok { + err = fmt.Errorf(ERRORMSG_CAST_ERROOR, err) + return + } + detailedResponse = castErr.Response + err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, castErr.Error()) + return } // Invoke the request. diff --git a/v4/core/base_service_test.go b/v4/core/base_service_test.go index 846aef3..3accc21 100644 --- a/v4/core/base_service_test.go +++ b/v4/core/base_service_test.go @@ -893,7 +893,6 @@ func TestIAMFailure(t *testing.T) { assert.NotNil(t, detailedResponse.GetHeaders()) assert.NotNil(t, detailedResponse.GetRawResult()) statusCode := detailedResponse.GetStatusCode() - assert.NotNil(t, statusCode) assert.Equal(t, http.StatusForbidden, statusCode) assert.Contains(t, err.Error(), "Sorry you are forbidden") } @@ -931,7 +930,6 @@ func TestIAMFailureRetryAfter(t *testing.T) { assert.NotNil(t, detailedResponse.GetRawResult()) statusCode := detailedResponse.GetStatusCode() headers := detailedResponse.GetHeaders() - assert.NotNil(t, statusCode) assert.NotNil(t, headers) assert.Equal(t, http.StatusTooManyRequests, statusCode) assert.Contains(t, headers, "Retry-After") @@ -1104,7 +1102,6 @@ func TestCP4DFail(t *testing.T) { assert.NotNil(t, detailedResponse.GetHeaders()) assert.NotNil(t, detailedResponse.GetRawResult()) statusCode := detailedResponse.GetStatusCode() - assert.NotNil(t, statusCode) assert.Equal(t, http.StatusForbidden, statusCode) assert.Contains(t, err.Error(), "Sorry you are forbidden") } @@ -1143,7 +1140,6 @@ func TestCp4dFailureRetryAfter(t *testing.T) { assert.NotNil(t, detailedResponse.GetRawResult()) statusCode := detailedResponse.GetStatusCode() headers := detailedResponse.GetHeaders() - assert.NotNil(t, statusCode) assert.NotNil(t, headers) assert.Equal(t, http.StatusTooManyRequests, statusCode) assert.Contains(t, headers, "Retry-After") diff --git a/v4/core/basic_authenticator.go b/v4/core/basic_authenticator.go index f939341..460f3ef 100644 --- a/v4/core/basic_authenticator.go +++ b/v4/core/basic_authenticator.go @@ -64,7 +64,7 @@ func (BasicAuthenticator) AuthenticationType() string { // // Authorization: Basic // -func (this *BasicAuthenticator) Authenticate(request *http.Request) *AuthenticationError { +func (this *BasicAuthenticator) Authenticate(request *http.Request) error { request.SetBasicAuth(this.Username, this.Password) return nil } diff --git a/v4/core/bearer_token_authenticator.go b/v4/core/bearer_token_authenticator.go index 4342dc6..8f2f35f 100644 --- a/v4/core/bearer_token_authenticator.go +++ b/v4/core/bearer_token_authenticator.go @@ -61,7 +61,7 @@ func (BearerTokenAuthenticator) AuthenticationType() string { // // Authorization: Bearer // -func (this *BearerTokenAuthenticator) Authenticate(request *http.Request) *AuthenticationError { +func (this *BearerTokenAuthenticator) Authenticate(request *http.Request) error { request.Header.Set("Authorization", fmt.Sprintf(`Bearer %s`, this.BearerToken)) return nil } diff --git a/v4/core/config_utils_test.go b/v4/core/config_utils_test.go index 2c3f96d..6e4779a 100644 --- a/v4/core/config_utils_test.go +++ b/v4/core/config_utils_test.go @@ -46,8 +46,8 @@ var testEnvironment = map[string]string{ "SERVICE3_USERNAME": "my-cp4d-user", "SERVICE3_PASSWORD": "my-cp4d-password", "SERVICE3_AUTH_DISABLE_SSL": "false", - "EQUAL_SERVICE_URL": "https://my=host.com/my=service/api", - "EQUAL_SERVICE_APIKEY": "===my=iam=apikey===", + "EQUAL_SERVICE_URL": "https://my=host.com/my=service/api", + "EQUAL_SERVICE_APIKEY": "===my=iam=apikey===", } // Set the environment variables described in our map. @@ -179,7 +179,6 @@ func TestGetServicePropertiesFromEnvironment(t *testing.T) { assert.NotNil(t, props) assert.Equal(t, "https://my=host.com/my=service/api", props[PROPNAME_SVC_URL]) assert.Equal(t, "===my=iam=apikey===", props[PROPNAME_APIKEY]) - props, err = getServiceProperties("not_a_service") assert.Nil(t, err) diff --git a/v4/core/constants.go b/v4/core/constants.go index d16aa64..59a71eb 100644 --- a/v4/core/constants.go +++ b/v4/core/constants.go @@ -56,4 +56,5 @@ const ( ERRORMSG_AUTHENTICATE_ERROR = "An error occurred while performing the 'authenticate' step: %s" ERRORMSG_READ_RESPONSE_BODY = "An error occurred while reading the response body: %s" ERRORMSG_UNMARSHAL_RESPONSE_BODY = "An error occurred while unmarshalling the response body: %s" + ERRORMSG_CAST_ERROOR = "An error occurred while casting to AuthenticationError type. Value: %s" ) diff --git a/v4/core/cp4d_authenticator.go b/v4/core/cp4d_authenticator.go index e533e10..983e150 100644 --- a/v4/core/cp4d_authenticator.go +++ b/v4/core/cp4d_authenticator.go @@ -137,7 +137,7 @@ func (authenticator CloudPakForDataAuthenticator) Validate() error { // // Authorization: Bearer // -func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Request) *AuthenticationError { +func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Request) error { token, err := authenticator.getToken() if err != nil { return err diff --git a/v4/core/cp4d_authenticator_test.go b/v4/core/cp4d_authenticator_test.go index a4db1a1..f9161cb 100644 --- a/v4/core/cp4d_authenticator_test.go +++ b/v4/core/cp4d_authenticator_test.go @@ -40,6 +40,39 @@ func TestCp4dConfigErrors(t *testing.T) { assert.NotNil(t, err) } +func TestCp4dAuthenticateFail(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte("Sorry you are not authorized")) + })) + defer server.Close() + + authenticator, err := NewCloudPakForDataAuthenticator(server.URL, "mookie", "betts", false, nil) + assert.Nil(t, err) + assert.NotNil(t, authenticator) + assert.Equal(t, authenticator.AuthenticationType(), AUTHTYPE_CP4D) + + // Create a new Request object. + builder, err := NewRequestBuilder("GET").ConstructHTTPURL("https://localhost/placeholder/url", nil, nil) + assert.Nil(t, err) + + request, err := builder.Build() + assert.Nil(t, err) + assert.NotNil(t, request) + + var authErr error + + authErr = authenticator.Authenticate(request) + // Validate the resulting error is a valid + assert.NotNil(t, authErr) + castErr, ok := authErr.(*AuthenticationError) + assert.True(t, ok) + assert.NotNil(t, castErr) + assert.EqualValues(t, authErr, castErr) + // The casted error should match the original error message + assert.Equal(t, authErr.Error(), castErr.Error()) +} + func TestCp4dGetTokenSuccess(t *testing.T) { firstCall := true server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/v4/core/detailed_response.go b/v4/core/detailed_response.go index fdd7490..4100204 100644 --- a/v4/core/detailed_response.go +++ b/v4/core/detailed_response.go @@ -42,7 +42,7 @@ type DetailedResponse struct { // objects. // // If the operation was successful and the response body contains a non-JSON response, - // the Result field will be an instance of io.ReadCloser that can be used by generated SDK code + // the Result field will be an instance of io.ReadCloser that can be used by generated SDK code // (or the application) to read the response data. // // If the operation was unsuccessful and the response body contains a JSON error response, diff --git a/v4/core/iam_authenticator.go b/v4/core/iam_authenticator.go index c3ccc4f..8bfd382 100644 --- a/v4/core/iam_authenticator.go +++ b/v4/core/iam_authenticator.go @@ -129,7 +129,7 @@ func (IamAuthenticator) AuthenticationType() string { // // Authorization: Bearer // -func (authenticator *IamAuthenticator) Authenticate(request *http.Request) *AuthenticationError { +func (authenticator *IamAuthenticator) Authenticate(request *http.Request) error { token, err := authenticator.getToken() if err != nil { return err diff --git a/v4/core/iam_authenticator_test.go b/v4/core/iam_authenticator_test.go index da0675e..2ed507a 100644 --- a/v4/core/iam_authenticator_test.go +++ b/v4/core/iam_authenticator_test.go @@ -44,6 +44,39 @@ func TestIamConfigErrors(t *testing.T) { assert.NotNil(t, err) } +func TestIamAuthenticateFail(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte("Sorry you are not authorized")) + })) + defer server.Close() + + authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) + assert.Nil(t, err) + assert.NotNil(t, authenticator) + assert.Equal(t, authenticator.AuthenticationType(), AUTHTYPE_IAM) + + // Create a new Request object. + builder, err := NewRequestBuilder("GET").ConstructHTTPURL("https://localhost/placeholder/url", nil, nil) + assert.Nil(t, err) + + request, err := builder.Build() + assert.Nil(t, err) + assert.NotNil(t, request) + + var authErr error + + authErr = authenticator.Authenticate(request) + // Validate the resulting error is a valid + assert.NotNil(t, authErr) + castErr, ok := authErr.(*AuthenticationError) + assert.True(t, ok) + assert.NotNil(t, castErr) + assert.EqualValues(t, authErr, castErr) + // The casted error should match the original error message + assert.Equal(t, authErr.Error(), castErr.Error()) +} + func TestIamGetTokenSuccess(t *testing.T) { firstCall := true server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/v4/core/marshal_nulls_test.go b/v4/core/marshal_nulls_test.go index b4bacf0..ef4a36d 100644 --- a/v4/core/marshal_nulls_test.go +++ b/v4/core/marshal_nulls_test.go @@ -8,7 +8,6 @@ import ( "testing" ) - // // The purpose of this testcase is to ensure that dynamic properties with nil values are // correctly serialized as JSON null values. @@ -23,8 +22,8 @@ import ( // serve as a test of serialize null dynamic property values. type dynamicModel struct { - Prop1 *string `json:"prop1,omitempty"` - Prop2 *int64 `json:"prop2,omitempty"` + Prop1 *string `json:"prop1,omitempty"` + Prop2 *int64 `json:"prop2,omitempty"` additionalProperties map[string]*string } @@ -88,14 +87,14 @@ func TestAdditionalPropertiesNull(t *testing.T) { Prop2: Int64Ptr(38), } model.SetProperty("bar", nil) - + // Serialize to JSON and ensure that the nil dynamic property value was explicitly serialized as JSON null. b, err := json.Marshal(model) jsonString := string(b) assert.Nil(t, err) t.Logf("Serialized model: %s\n", jsonString) assert.Contains(t, jsonString, `"bar":null`) - + // Next, deserialize the json string into a map of RawMessages to simulate how the SDK code will // deserialize a response body. var rawMap map[string]json.RawMessage @@ -109,7 +108,7 @@ func TestAdditionalPropertiesNull(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, newModel) t.Logf("newModel: %+v\n", *newModel) - + // Make sure the new model is the same as the original model. assert.Equal(t, model, newModel) } diff --git a/v4/core/noauth_authenticator.go b/v4/core/noauth_authenticator.go index 1d19493..f77565a 100644 --- a/v4/core/noauth_authenticator.go +++ b/v4/core/noauth_authenticator.go @@ -35,7 +35,7 @@ func (NoAuthAuthenticator) Validate() error { return nil } -func (this *NoAuthAuthenticator) Authenticate(request *http.Request) *AuthenticationError { +func (this *NoAuthAuthenticator) Authenticate(request *http.Request) error { // Nothing to do since we're not providing any authentication. return nil } diff --git a/v4/core/unmarshal_v2.go b/v4/core/unmarshal_v2.go index f90de5f..aa71041 100644 --- a/v4/core/unmarshal_v2.go +++ b/v4/core/unmarshal_v2.go @@ -478,7 +478,7 @@ func unmarshalModelSliceMap(rawInput interface{}, propertyName string, result in if foundInput && rawMap != nil { for k, v := range rawMap { - + // Make sure our slice raw message isn't an explicit JSON null value. if !isJsonNull(v) { // Each value in 'rawMap' should contain an instance of []. diff --git a/v4/core/unmarshal_v2_primitives_test.go b/v4/core/unmarshal_v2_primitives_test.go index 13c063f..1d82192 100644 --- a/v4/core/unmarshal_v2_primitives_test.go +++ b/v4/core/unmarshal_v2_primitives_test.go @@ -152,7 +152,7 @@ func TestUnmarshalPrimitiveString(t *testing.T) { assert.NotNil(t, err) assert.True(t, strings.Contains(err.Error(), "error unmarshalling property 'bad_slice_type'")) t.Logf("Expected error: %s\n", err.Error()) - + err = UnmarshalPrimitive(rawMap, "", &model.Prop) assert.NotNil(t, err) assert.Equal(t, "the 'propertyName' parameter is required", err.Error()) @@ -979,7 +979,7 @@ func TestUnmarshalPrimitiveDateTime(t *testing.T) { jsonString = strings.ReplaceAll(jsonString, "%d2", d2) jsonString = strings.ReplaceAll(jsonString, "%d3", d3) jsonString = strings.ReplaceAll(jsonString, "%d4", d4) - + // Expected values need to include ms d1 = "1969-07-20T20:17:00.000Z" d2 = "1963-11-22T18:30:00.000Z" @@ -1329,7 +1329,7 @@ func TestUnmarshalPrimitiveAny(t *testing.T) { assert.Nil(t, err) assert.Nil(t, model.PropSlice) - model.PropMap = map[string]interface{}{ "key1": "value1" } + model.PropMap = map[string]interface{}{"key1": "value1"} err = UnmarshalPrimitive(rawMap, "null_prop", &model.PropMap) assert.Nil(t, err) assert.Nil(t, model.PropMap) @@ -1386,7 +1386,7 @@ func TestUnmarshalPrimitiveObject(t *testing.T) { "bad_slice_type": [38, 26], "null_prop": null }` - + o1 := `{"field1": "value1"}` o2 := `{"field2": "value2"}`