Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
jorge-ibm committed Aug 12, 2020
1 parent 8147adc commit a9f0a25
Show file tree
Hide file tree
Showing 16 changed files with 90 additions and 27 deletions.
2 changes: 1 addition & 1 deletion v4/core/authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
10 changes: 8 additions & 2 deletions v4/core/base_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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_ERROR, err)
return
}
detailedResponse = castErr.Response
err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, castErr.Error())
return
}

// Invoke the request.
Expand Down
4 changes: 0 additions & 4 deletions v4/core/base_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
Expand Down
2 changes: 1 addition & 1 deletion v4/core/basic_authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (BasicAuthenticator) AuthenticationType() string {
//
// Authorization: Basic <encoded username and password>
//
func (this *BasicAuthenticator) Authenticate(request *http.Request) *AuthenticationError {
func (this *BasicAuthenticator) Authenticate(request *http.Request) error {
request.SetBasicAuth(this.Username, this.Password)
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion v4/core/bearer_token_authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func (BearerTokenAuthenticator) AuthenticationType() string {
//
// Authorization: Bearer <bearer-token>
//
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
}
Expand Down
5 changes: 2 additions & 3 deletions v4/core/config_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions v4/core/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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_ERROR = "An error occurred while casting to AuthenticationError type. Value: %s"
)
2 changes: 1 addition & 1 deletion v4/core/cp4d_authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func (authenticator CloudPakForDataAuthenticator) Validate() error {
//
// Authorization: Bearer <bearer-token>
//
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
Expand Down
31 changes: 31 additions & 0 deletions v4/core/cp4d_authenticator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,37 @@ 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)

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) {
Expand Down
2 changes: 1 addition & 1 deletion v4/core/detailed_response.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion v4/core/iam_authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func (IamAuthenticator) AuthenticationType() string {
//
// Authorization: Bearer <bearer-token>
//
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
Expand Down
31 changes: 31 additions & 0 deletions v4/core/iam_authenticator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,37 @@ 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)

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) {
Expand Down
11 changes: 5 additions & 6 deletions v4/core/marshal_nulls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand All @@ -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)
}
2 changes: 1 addition & 1 deletion v4/core/noauth_authenticator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion v4/core/unmarshal_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 []<model>.
Expand Down
8 changes: 4 additions & 4 deletions v4/core/unmarshal_v2_primitives_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -1386,7 +1386,7 @@ func TestUnmarshalPrimitiveObject(t *testing.T) {
"bad_slice_type": [38, 26],
"null_prop": null
}`

o1 := `{"field1": "value1"}`
o2 := `{"field2": "value2"}`

Expand Down

0 comments on commit a9f0a25

Please sign in to comment.