feat: add detailed error response to iam/cp4d authenticators #66

jorge-ibm · 2020-08-10T19:14:27Z

This PR creates a DetailedResponse instance for both the IAM/CP4D authenticators in the case where the api call to either servers fails (returns a status code we consider a failure). This DetailedResponse instance contains the response status code, headers, and the raw byte result.

I've created a AuthenticationError struct that contains an instance of DetailedResponse along with the err field which contains the actual error object. Whenever we expect a detailed response to be returned from the Authenticate method due to an error, we attempt to cast the returned error into AuthenticationError type. If successful, the detailed response is included in the returned value for the Base Service's Request method.

ref: https://github.ibm.com/arf/planning-sdk-squad/issues/2030

jorge-ibm · 2020-08-11T16:41:04Z

After some discussion with @padamstx, there's some needed refactoring on my set of changes. I will re-request reviews once those changes are done.

mkistler · 2020-08-12T03:42:14Z

This looks like it will be a breaking change, since it changes an external interface. If that's correct, we just need to make sure we include the right markers in the commit message so that semantic release does the right thing.

padamstx · 2020-08-12T14:24:39Z

This looks like it will be a breaking change, since it changes an external interface. If that's correct, we just need to make sure we include the right markers in the commit message so that semantic release does the right thing.

Agree... I hadn't considered this angle yet, but we do in fact have code other than the Go core itself that explicitly calls the Authenticate() method, so we'll need to package this as a new major version.

@jorge-ibm To do this, we'll need to do two things:

Change the commit message to include the BREAKING CHANGE marker in the body of the commit message.
We'll need to rename the top-level v4 directory to be v5 and modify .bumpversion.cfg to reflect the new directory name

Then once this change has been merged in, we'll need to change the generator to use "v5" in the import path used for the core.
Let's discuss so we can coordinate this.

Edit: Actually, I just thought of a possible way to avoid a new major version in this situation...
Because the new AuthenticationError struct implements the "error" interface, I think we can change the return type back to just "error" and other code using Authenticate() can continue thinking that it is just a simple error object on which they can call the Error() method. When the BaseService.Request() method detects a non-nil error object returned by Authenticate() it could do a type assertion to "cast" it as an AuthenticationError and then extract the DetailedResponse field, then return it along with the error message to complete the error processing.

padamstx

I think we can avoid a breaking change by keeping the return type as error for the Authenticate() method, even though it will be returning an AuthenticationError instance. This is because the AuthenticationError struct implements the "error" interface.

Please make sure that your tests include at least one scenario where we're explicitly calling Authenticate() and receiving the result as a "error".

padamstx · 2020-08-12T16:00:02Z

v4/core/authenticator.go

@@ -21,6 +21,16 @@ import (
 // Authenticator describes the set of methods implemented by each authenticator.
 type Authenticator interface {
 	AuthenticationType() string
-	Authenticate(*http.Request) error
+	Authenticate(*http.Request) *AuthenticationError


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 :)

padamstx · 2020-08-12T16:29:56Z

v4/core/base_service.go

-		return
+	authError := service.Options.Authenticator.Authenticate(req)
+	if authError != nil {
+		err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, authError.Error())


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 castErr in this example), then set detailedResponse = castErr.Response.

padamstx · 2020-08-12T16:30:51Z

v4/core/base_service.go

+	authError := service.Options.Authenticator.Authenticate(req)
+	if authError != nil {
+		err = fmt.Errorf(ERRORMSG_AUTHENTICATE_ERROR, authError.Error())
+		return authError.Response, err


We just need to make sure that the detailedResponse and err named return values are assigned their correct values, then just do a naked return, like before.

Suggested change

return authError.Response, err

return

padamstx · 2020-08-12T16:32:26Z

v4/core/base_service_test.go

+	assert.NotNil(t, detailedResponse.GetHeaders())
+	assert.NotNil(t, detailedResponse.GetRawResult())
+	statusCode := detailedResponse.GetStatusCode()
+	assert.NotNil(t, statusCode)


statusCode is an int, so you can't really check it against nil.

padamstx · 2020-08-12T16:32:46Z

v4/core/base_service_test.go

+	assert.NotNil(t, detailedResponse.GetRawResult())
+	statusCode := detailedResponse.GetStatusCode()
+	headers := detailedResponse.GetHeaders()
+	assert.NotNil(t, statusCode)


padamstx · 2020-08-12T16:32:58Z

v4/core/base_service_test.go

+	assert.NotNil(t, detailedResponse.GetHeaders())
+	assert.NotNil(t, detailedResponse.GetRawResult())
+	statusCode := detailedResponse.GetStatusCode()
+	assert.NotNil(t, statusCode)


padamstx · 2020-08-12T16:33:10Z

v4/core/base_service_test.go

+	assert.NotNil(t, detailedResponse.GetRawResult())
+	statusCode := detailedResponse.GetStatusCode()
+	headers := detailedResponse.GetHeaders()
+	assert.NotNil(t, statusCode)


jorge-ibm · 2020-08-12T19:43:05Z

v4/core/base_service.go

+	if authError != nil {
+		castErr, ok := authError.(*AuthenticationError)
+		if !ok {
+			err = fmt.Errorf(ERRORMSG_CAST_ERROR, err)


Casting technically shouldn't fail, but just in case.

I think we probably don't need to worry too much about the !ok case and return a different error in that scenario, because that would cause us to lose the actual authentication error.
So perhaps we could do something like this, which essentially is "return the DetailedResponse if we can":

if authError != nil { err = authError.Error() castErr, ok := authError.(*AuthenticationError) if ok { detailedResponse = castErr.Response } return }

jorge-ibm · 2020-08-12T19:44:08Z

v4/core/cp4d_authenticator_test.go

@@ -40,6 +40,39 @@ func TestCp4dConfigErrors(t *testing.T) {
 	assert.NotNil(t, err)
 }

+func TestCp4dAuthenticateFail(t *testing.T) {


Adding this new test to test the error returned from the Authenticate method

jorge-ibm · 2020-08-12T19:51:45Z

@padamstx @mkistler I've pushed a commit addressing the review comments along with running go fmt ./... to auto-format some code.

padamstx

Jorge, first of all I apologize for not giving you better direction initially and I'm sure this PR seems like a wild goose chase at this point. Having said that I think there are a few more changes that would make this a little cleaner:

As much as possible, let's try to use error as the return type of various functions instead of *AuthenticationError. I think we can use error for all function return types since AuthenticationError
implements the Error() function, but I'm open to being convinced otherwise :)
Let's avoid creating any error structs ahead of time and just create them when/if we need to actually return an error.
By reverting all function return types back to using error, that means that we would need to return an actual instance of AuthenticationError ONLY if we also wanted to return an instance of DetailedResponse along with the error message. In situations where we will not be returning a DetailedResponse, we could simply return an error object instead.

I think these changes will make it cleaner and reduce the "blast radius" a bit.

padamstx · 2020-08-12T22:55:11Z

v4/core/base_service.go

+	if authError != nil {
+		castErr, ok := authError.(*AuthenticationError)
+		if !ok {
+			err = fmt.Errorf(ERRORMSG_CAST_ERROR, err)


I think we probably don't need to worry too much about the !ok case and return a different error in that scenario, because that would cause us to lose the actual authentication error.
So perhaps we could do something like this, which essentially is "return the DetailedResponse if we can":

if authError != nil { err = authError.Error() castErr, ok := authError.(*AuthenticationError) if ok { detailedResponse = castErr.Response } return }

padamstx · 2020-08-12T22:56:39Z

v4/core/base_service_test.go

+	assert.NotNil(t, detailedResponse.GetHeaders())
+	assert.NotNil(t, detailedResponse.GetRawResult())
+	statusCode := detailedResponse.GetStatusCode()
+	assert.Equal(t, http.StatusForbidden, statusCode)


padamstx · 2020-08-12T22:58:27Z

v4/core/cp4d_authenticator.go

@@ -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) {


I think we can revert this back to just error now that AuthenticationError implements the error interface.

padamstx · 2020-08-12T22:58:45Z

v4/core/cp4d_authenticator.go

@@ -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)


I think we can also revert this back to error as well.

padamstx · 2020-08-12T23:01:13Z

v4/core/cp4d_authenticator.go

@@ -172,9 +172,13 @@ func (authenticator *CloudPakForDataAuthenticator) getToken() (string, error) {
 		}
 	}

+	authError := &AuthenticationError{}


I think we should initialize authError only if we're going to return an actual error.

Suggested change

authError := &AuthenticationError{}

padamstx

Looks great!
I'm glad we were able to make this change without changing the return types in function signatures.

# [4.2.0](v4.1.0...v4.2.0) (2020-08-14) ### Features * add detailed error response to iam/cp4d authenticators ([#66](#66)) ([3485263](3485263))

ibm-devx-automation · 2020-08-14T15:18:42Z

🎉 This PR is included in version 4.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

jorge-ibm requested review from padamstx, mkistler and dpopp07 August 10, 2020 19:14

jorge-ibm added ready for review and removed ready for review labels Aug 11, 2020

jorge-ibm removed request for mkistler, dpopp07 and padamstx August 11, 2020 16:41

jorge-ibm force-pushed the auth/add-detailed-response branch from e906eb3 to 77c75b6 Compare August 11, 2020 19:29

feat: add detailed error response to iam/cp4d authenticators

8147adc

jorge-ibm force-pushed the auth/add-detailed-response branch from 77c75b6 to 8147adc Compare August 11, 2020 19:37

jorge-ibm requested review from padamstx, mkistler and dpopp07 August 11, 2020 19:38

jorge-ibm added the ready for review label Aug 11, 2020

padamstx requested changes Aug 12, 2020

View reviewed changes

jorge-ibm force-pushed the auth/add-detailed-response branch 3 times, most recently from d89fc2c to c7e1b32 Compare August 12, 2020 19:42

jorge-ibm commented Aug 12, 2020

View reviewed changes

jorge-ibm requested a review from padamstx August 12, 2020 19:46

address review comments

a9f0a25

jorge-ibm force-pushed the auth/add-detailed-response branch from c7e1b32 to a9f0a25 Compare August 12, 2020 19:48

padamstx requested changes Aug 12, 2020

View reviewed changes

more review changes

40f0b71

jorge-ibm force-pushed the auth/add-detailed-response branch from b3e92c9 to 40f0b71 Compare August 13, 2020 18:59

jorge-ibm requested a review from padamstx August 13, 2020 19:05

padamstx approved these changes Aug 13, 2020

View reviewed changes

jorge-ibm merged commit 3485263 into master Aug 14, 2020

ibm-devx-automation pushed a commit that referenced this pull request Aug 14, 2020

chore(release): 4.2.0 release notes [skip ci]

ed7e12e

# [4.2.0](v4.1.0...v4.2.0) (2020-08-14) ### Features * add detailed error response to iam/cp4d authenticators ([#66](#66)) ([3485263](3485263))

ibm-devx-automation added the released label Aug 14, 2020

padamstx deleted the auth/add-detailed-response branch October 14, 2020 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add detailed error response to iam/cp4d authenticators #66

feat: add detailed error response to iam/cp4d authenticators #66

jorge-ibm commented Aug 10, 2020 •

edited

Loading

jorge-ibm commented Aug 11, 2020

mkistler commented Aug 12, 2020

padamstx commented Aug 12, 2020 •

edited

Loading

padamstx left a comment •

edited

Loading

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

jorge-ibm Aug 12, 2020 •

edited

Loading

padamstx Aug 12, 2020

jorge-ibm Aug 12, 2020

jorge-ibm commented Aug 12, 2020

padamstx left a comment

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx Aug 12, 2020

padamstx left a comment

ibm-devx-automation commented Aug 14, 2020

feat: add detailed error response to iam/cp4d authenticators #66

feat: add detailed error response to iam/cp4d authenticators #66

Conversation

jorge-ibm commented Aug 10, 2020 • edited Loading

jorge-ibm commented Aug 11, 2020

mkistler commented Aug 12, 2020

padamstx commented Aug 12, 2020 • edited Loading

padamstx left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorge-ibm Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorge-ibm commented Aug 12, 2020

padamstx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

ibm-devx-automation commented Aug 14, 2020

jorge-ibm commented Aug 10, 2020 •

edited

Loading

padamstx commented Aug 12, 2020 •

edited

Loading

padamstx left a comment •

edited

Loading

jorge-ibm Aug 12, 2020 •

edited

Loading