-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add detailed error response to iam/cp4d authenticators #66
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -219,10 +219,10 @@ func (service *BaseService) Request(req *http.Request, result interface{}) (deta | |||||
return | ||||||
} | ||||||
|
||||||
err = service.Options.Authenticator.Authenticate(req) | ||||||
if err != nil { | ||||||
err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, err.Error()) | ||||||
return | ||||||
authError := service.Options.Authenticator.Authenticate(req) | ||||||
if authError != nil { | ||||||
err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, authError.Error()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After initializing "err" on this line, you'd need to do a type assertion to cast "authErr" to be an AuthenticationError instance (we'll use |
||||||
return authError.Response, err | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We just need to make sure that the
Suggested change
|
||||||
} | ||||||
|
||||||
// Invoke the request. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,7 +144,7 @@ func TestGoodResponseJSONExtraFields(t *testing.T) { | |
Authenticator: &NoAuthAuthenticator{}, | ||
} | ||
service, _ := NewBaseService(options) | ||
|
||
var foo *Foo | ||
detailedResponse, _ := service.Request(req, &foo) | ||
result, ok := detailedResponse.Result.(*Foo) | ||
|
@@ -254,7 +254,7 @@ func TestGoodResponseString(t *testing.T) { | |
assert.Nil(t, err) | ||
assert.NotNil(t, service.Options.Authenticator) | ||
assert.Equal(t, AUTHTYPE_NOAUTH, service.Options.Authenticator.AuthenticationType()) | ||
|
||
var responseString *string | ||
detailedResponse, err := service.Request(req, &responseString) | ||
assert.Nil(t, err) | ||
|
@@ -264,7 +264,7 @@ func TestGoodResponseString(t *testing.T) { | |
assert.NotNil(t, detailedResponse.Result) | ||
assert.NotNil(t, responseString) | ||
assert.Equal(t, expectedResponse, *responseString) | ||
|
||
resultField, ok := detailedResponse.Result.(*string) | ||
assert.Equal(t, true, ok) | ||
assert.NotNil(t, resultField) | ||
|
@@ -330,7 +330,7 @@ func TestGoodResponseJSONDeserFailure(t *testing.T) { | |
Authenticator: &NoAuthAuthenticator{}, | ||
} | ||
service, _ := NewBaseService(options) | ||
|
||
var foo *Foo | ||
detailedResponse, err := service.Request(req, &foo) | ||
assert.NotNil(t, detailedResponse) | ||
|
@@ -437,7 +437,7 @@ func TestErrorResponseJSON(t *testing.T) { | |
Authenticator: &NoAuthAuthenticator{}, | ||
} | ||
service, _ := NewBaseService(options) | ||
|
||
var foo *Foo | ||
response, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
|
@@ -473,7 +473,7 @@ func TestErrorResponseJSONDeserError(t *testing.T) { | |
Authenticator: &NoAuthAuthenticator{}, | ||
} | ||
service, _ := NewBaseService(options) | ||
|
||
var foo *Foo | ||
response, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
|
@@ -505,7 +505,7 @@ func TestErrorResponseNotJSON(t *testing.T) { | |
Authenticator: &NoAuthAuthenticator{}, | ||
} | ||
service, _ := NewBaseService(options) | ||
|
||
var foo *Foo | ||
response, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
|
@@ -585,7 +585,7 @@ func TestRequestForDefaultUserAgent(t *testing.T) { | |
Authenticator: authenticator, | ||
} | ||
service, _ := NewBaseService(options) | ||
|
||
var foo *Foo | ||
_, _ = service.Request(req, &foo) | ||
} | ||
|
@@ -614,7 +614,7 @@ func TestRequestForProvidedUserAgent(t *testing.T) { | |
headers := http.Header{} | ||
headers.Add("User-Agent", "provided user agent") | ||
service.SetDefaultHeaders(headers) | ||
|
||
var foo *Foo | ||
_, _ = service.Request(req, &foo) | ||
} | ||
|
@@ -887,11 +887,57 @@ func TestIAMFailure(t *testing.T) { | |
assert.NotNil(t, service.Options.Authenticator) | ||
|
||
var foo *Foo | ||
_, err = service.Request(req, &foo) | ||
detailedResponse, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
assert.NotNil(t, detailedResponse) | ||
assert.NotNil(t, detailedResponse.GetHeaders()) | ||
assert.NotNil(t, detailedResponse.GetRawResult()) | ||
statusCode := detailedResponse.GetStatusCode() | ||
assert.NotNil(t, statusCode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. statusCode is an int, so you can't really check it against |
||
assert.Equal(t, http.StatusForbidden, statusCode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
assert.Contains(t, err.Error(), "Sorry you are forbidden") | ||
} | ||
|
||
func TestIAMFailureRetryAfter(t *testing.T) { | ||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Header().Set("Retry-After", "20") | ||
w.WriteHeader(http.StatusTooManyRequests) | ||
_, _ = w.Write([]byte("Sorry rate limit has been exceeded")) | ||
})) | ||
defer server.Close() | ||
|
||
builder := NewRequestBuilder("GET") | ||
_, err := builder.ConstructHTTPURL(server.URL, nil, nil) | ||
assert.Nil(t, err) | ||
builder.AddQuery("Version", "2018-22-09") | ||
req, _ := builder.Build() | ||
|
||
options := &ServiceOptions{ | ||
URL: server.URL, | ||
Authenticator: &IamAuthenticator{ | ||
URL: server.URL, | ||
ApiKey: "xxxxx", | ||
}, | ||
} | ||
service, err := NewBaseService(options) | ||
assert.Nil(t, err) | ||
assert.NotNil(t, service) | ||
assert.NotNil(t, service.Options.Authenticator) | ||
|
||
var foo *Foo | ||
detailedResponse, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
assert.NotNil(t, detailedResponse) | ||
assert.NotNil(t, detailedResponse.GetRawResult()) | ||
statusCode := detailedResponse.GetStatusCode() | ||
headers := detailedResponse.GetHeaders() | ||
assert.NotNil(t, statusCode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
assert.NotNil(t, headers) | ||
assert.Equal(t, http.StatusTooManyRequests, statusCode) | ||
assert.Contains(t, headers, "Retry-After") | ||
assert.Contains(t, err.Error(), "Sorry rate limit has been exceeded") | ||
} | ||
|
||
func TestIAMWithIdSecret(t *testing.T) { | ||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.WriteHeader(http.StatusOK) | ||
|
@@ -1052,10 +1098,58 @@ func TestCP4DFail(t *testing.T) { | |
assert.NotNil(t, service.Options.Authenticator) | ||
|
||
var foo *Foo | ||
_, err = service.Request(req, &foo) | ||
detailedResponse, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
assert.NotNil(t, detailedResponse) | ||
assert.NotNil(t, detailedResponse.GetHeaders()) | ||
assert.NotNil(t, detailedResponse.GetRawResult()) | ||
statusCode := detailedResponse.GetStatusCode() | ||
assert.NotNil(t, statusCode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
assert.Equal(t, http.StatusForbidden, statusCode) | ||
assert.Contains(t, err.Error(), "Sorry you are forbidden") | ||
} | ||
|
||
func TestCp4dFailureRetryAfter(t *testing.T) { | ||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Header().Set("Retry-After", "20") | ||
w.WriteHeader(http.StatusTooManyRequests) | ||
_, _ = w.Write([]byte("Sorry rate limit has been exceeded")) | ||
})) | ||
defer server.Close() | ||
|
||
builder := NewRequestBuilder("GET") | ||
_, err := builder.ConstructHTTPURL(server.URL, nil, nil) | ||
assert.Nil(t, err) | ||
builder.AddQuery("Version", "2018-22-09") | ||
req, _ := builder.Build() | ||
|
||
options := &ServiceOptions{ | ||
URL: server.URL, | ||
Authenticator: &CloudPakForDataAuthenticator{ | ||
URL: server.URL, | ||
Username: "bogus", | ||
Password: "bogus", | ||
}, | ||
} | ||
service, err := NewBaseService(options) | ||
assert.Nil(t, err) | ||
assert.NotNil(t, service) | ||
assert.NotNil(t, service.Options.Authenticator) | ||
|
||
var foo *Foo | ||
detailedResponse, err := service.Request(req, &foo) | ||
assert.NotNil(t, err) | ||
assert.NotNil(t, detailedResponse) | ||
assert.NotNil(t, detailedResponse.GetRawResult()) | ||
statusCode := detailedResponse.GetStatusCode() | ||
headers := detailedResponse.GetHeaders() | ||
assert.NotNil(t, statusCode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one more |
||
assert.NotNil(t, headers) | ||
assert.Equal(t, http.StatusTooManyRequests, statusCode) | ||
assert.Contains(t, headers, "Retry-After") | ||
assert.Contains(t, err.Error(), "Sorry rate limit has been exceeded") | ||
} | ||
|
||
// Test for the deprecated SetURL method. | ||
func TestSetURL(t *testing.T) { | ||
service, err := NewBaseService( | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -137,7 +137,7 @@ func (authenticator CloudPakForDataAuthenticator) Validate() error { | |||
// | ||||
// Authorization: Bearer <bearer-token> | ||||
// | ||||
func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Request) error { | ||||
func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Request) *AuthenticationError { | ||||
token, err := authenticator.getToken() | ||||
if err != nil { | ||||
return err | ||||
|
@@ -150,7 +150,7 @@ func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Re | |||
// getToken: returns an access token to be used in an Authorization header. | ||||
// Whenever a new token is needed (when a token doesn't yet exist, needs to be refreshed, | ||||
// or the existing token has expired), a new access token is fetched from the token server. | ||||
func (authenticator *CloudPakForDataAuthenticator) getToken() (string, error) { | ||||
func (authenticator *CloudPakForDataAuthenticator) getToken() (string, *AuthenticationError) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can revert this back to just |
||||
if authenticator.tokenData == nil || !authenticator.tokenData.isTokenValid() { | ||||
// synchronously request the token | ||||
err := authenticator.synchronizedRequestToken() | ||||
|
@@ -159,7 +159,7 @@ func (authenticator *CloudPakForDataAuthenticator) getToken() (string, error) { | |||
} | ||||
} else if authenticator.tokenData.needsRefresh() { | ||||
// If refresh needed, kick off a go routine in the background to get a new token | ||||
ch := make(chan error) | ||||
ch := make(chan *AuthenticationError) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can also revert this back to |
||||
go func() { | ||||
ch <- authenticator.getTokenData() | ||||
}() | ||||
|
@@ -172,9 +172,13 @@ func (authenticator *CloudPakForDataAuthenticator) getToken() (string, error) { | |||
} | ||||
} | ||||
|
||||
authError := &AuthenticationError{} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should initialize authError only if we're going to return an actual error.
Suggested change
|
||||
|
||||
// return an error if the access token is not valid or was not fetched | ||||
if authenticator.tokenData == nil || authenticator.tokenData.AccessToken == "" { | ||||
return "", fmt.Errorf("Error while trying to get access token") | ||||
errorMsg := fmt.Errorf("Error while trying to get access token") | ||||
authError.err = errorMsg | ||||
return "", authError | ||||
} | ||||
|
||||
return authenticator.tokenData.AccessToken, nil | ||||
|
@@ -183,7 +187,7 @@ func (authenticator *CloudPakForDataAuthenticator) getToken() (string, error) { | |||
// synchronizedRequestToken: synchronously checks if the current token in cache | ||||
// is valid. If token is not valid or does not exist, it will fetch a new token | ||||
// and set the tokenRefreshTime | ||||
func (authenticator *CloudPakForDataAuthenticator) synchronizedRequestToken() error { | ||||
func (authenticator *CloudPakForDataAuthenticator) synchronizedRequestToken() *AuthenticationError { | ||||
cp4dRequestTokenMutex.Lock() | ||||
defer cp4dRequestTokenMutex.Unlock() | ||||
// if cached token is still valid, then just continue to use it | ||||
|
@@ -197,32 +201,39 @@ func (authenticator *CloudPakForDataAuthenticator) synchronizedRequestToken() er | |||
// getTokenData: requests a new token from the access server and | ||||
// unmarshals the token information to the tokenData cache. Returns | ||||
// an error if the token was unable to be fetched, otherwise returns nil | ||||
func (authenticator *CloudPakForDataAuthenticator) getTokenData() error { | ||||
tokenResponse, err := authenticator.requestToken() | ||||
if err != nil { | ||||
return err | ||||
func (authenticator *CloudPakForDataAuthenticator) getTokenData() *AuthenticationError { | ||||
var err error | ||||
tokenResponse, authErr := authenticator.requestToken() | ||||
if authErr != nil { | ||||
return authErr | ||||
} | ||||
|
||||
newAuthenticationError := &AuthenticationError{} | ||||
|
||||
authenticator.tokenData, err = newCp4dTokenData(tokenResponse) | ||||
if err != nil { | ||||
return err | ||||
newAuthenticationError.err = err | ||||
return newAuthenticationError | ||||
} | ||||
|
||||
return nil | ||||
} | ||||
|
||||
// requestToken: fetches a new access token from the token server. | ||||
func (authenticator *CloudPakForDataAuthenticator) requestToken() (*cp4dTokenServerResponse, error) { | ||||
func (authenticator *CloudPakForDataAuthenticator) requestToken() (*cp4dTokenServerResponse, *AuthenticationError) { | ||||
// If the user-specified URL does not end with the required path, | ||||
// then add it now. | ||||
url := authenticator.URL | ||||
if !strings.HasSuffix(url, PRE_AUTH_PATH) { | ||||
url = fmt.Sprintf("%s%s", url, PRE_AUTH_PATH) | ||||
} | ||||
|
||||
authError := &AuthenticationError{} | ||||
|
||||
builder, err := NewRequestBuilder(GET).ConstructHTTPURL(url, nil, nil) | ||||
if err != nil { | ||||
return nil, err | ||||
authError.err = err | ||||
return nil, authError | ||||
} | ||||
|
||||
// Add user-defined headers to request. | ||||
|
@@ -232,7 +243,8 @@ func (authenticator *CloudPakForDataAuthenticator) requestToken() (*cp4dTokenSer | |||
|
||||
req, err := builder.Build() | ||||
if err != nil { | ||||
return nil, err | ||||
authError.err = err | ||||
return nil, authError | ||||
} | ||||
|
||||
req.SetBasicAuth(authenticator.Username, authenticator.Password) | ||||
|
@@ -254,14 +266,24 @@ func (authenticator *CloudPakForDataAuthenticator) requestToken() (*cp4dTokenSer | |||
|
||||
resp, err := authenticator.Client.Do(req) | ||||
if err != nil { | ||||
return nil, err | ||||
authError.err = err | ||||
return nil, authError | ||||
} | ||||
|
||||
// Start to populate the DetailedResponse. | ||||
detailedResponse := &DetailedResponse{ | ||||
StatusCode: resp.StatusCode, | ||||
Headers: resp.Header, | ||||
} | ||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||||
if resp != nil { | ||||
buff := new(bytes.Buffer) | ||||
_, _ = buff.ReadFrom(resp.Body) | ||||
return nil, fmt.Errorf(buff.String()) | ||||
authError.err = fmt.Errorf(buff.String()) | ||||
detailedResponse.RawResult = buff.Bytes() | ||||
authError.Response = detailedResponse | ||||
return nil, authError | ||||
} | ||||
} | ||||
|
||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can revert this back to
error
as the return type in order to maintain compatibility in the API. This will mean that where we call Authenticate() in BaseService.Request(), you'll need to adjust that code to cast the "error" to an "AuthenticationError" when trying to retrieve the DetailedResponse field. I hadn't thought of this "trick" before, but should have :)