From b8f9cb3d3155673bfa99d47f4a520d8944d64a9a Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Mon, 22 Mar 2021 19:33:37 +0100 Subject: [PATCH 1/8] fix: make cp4d authenticator token data thread safe --- v5/core/cp4d_authenticator.go | 46 +++++++++++++----- v5/core/cp4d_authenticator_test.go | 78 +++++++++++++++--------------- 2 files changed, 72 insertions(+), 52 deletions(-) diff --git a/v5/core/cp4d_authenticator.go b/v5/core/cp4d_authenticator.go index 5fa2cf4..6fd9046 100644 --- a/v5/core/cp4d_authenticator.go +++ b/v5/core/cp4d_authenticator.go @@ -60,6 +60,9 @@ type CloudPakForDataAuthenticator struct { // The cached token and expiration time. tokenData *cp4dTokenData + + // Mutex to make the tokenData field thread safe. + tokenDataMutex sync.Mutex } var cp4dRequestTokenMutex sync.Mutex @@ -170,21 +173,37 @@ func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Re return nil } +// getTokenData returns the tokeData field from the authenticator. +func (authenticator *CloudPakForDataAuthenticator) getTokenData() *cp4dTokenData { + authenticator.tokenDataMutex.Lock() + defer authenticator.tokenDataMutex.Unlock() + + return authenticator.tokenData +} + +// setTokenData sets the given cp4dTokenData to the tokenData field of the authenticator. +func (authenticator *CloudPakForDataAuthenticator) setTokenData(tokenData *cp4dTokenData) { + authenticator.tokenDataMutex.Lock() + defer authenticator.tokenDataMutex.Unlock() + + authenticator.tokenData = tokenData +} + // 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) { - if authenticator.tokenData == nil || !authenticator.tokenData.isTokenValid() { + if authenticator.getTokenData() == nil || !authenticator.getTokenData().isTokenValid() { // synchronously request the token err := authenticator.synchronizedRequestToken() if err != nil { return "", err } - } else if authenticator.tokenData.needsRefresh() { + } else if authenticator.getTokenData().needsRefresh() { // If refresh needed, kick off a go routine in the background to get a new token ch := make(chan error) go func() { - ch <- authenticator.getTokenData() + ch <- authenticator.invokeRequestTokenData() }() select { case err := <-ch: @@ -196,11 +215,11 @@ func (authenticator *CloudPakForDataAuthenticator) getToken() (string, error) { } // return an error if the access token is not valid or was not fetched - if authenticator.tokenData == nil || authenticator.tokenData.AccessToken == "" { + if authenticator.getTokenData() == nil || authenticator.getTokenData().AccessToken == "" { return "", fmt.Errorf("Error while trying to get access token") } - return authenticator.tokenData.AccessToken, nil + return authenticator.getTokenData().AccessToken, nil } // synchronizedRequestToken: synchronously checks if the current token in cache @@ -210,27 +229,28 @@ func (authenticator *CloudPakForDataAuthenticator) synchronizedRequestToken() er cp4dRequestTokenMutex.Lock() defer cp4dRequestTokenMutex.Unlock() // if cached token is still valid, then just continue to use it - if authenticator.tokenData != nil && authenticator.tokenData.isTokenValid() { + if authenticator.getTokenData() != nil && authenticator.getTokenData().isTokenValid() { return nil } - return authenticator.getTokenData() + return authenticator.invokeRequestTokenData() } -// getTokenData: requests a new token from the token server and +// invokeRequestTokenData: requests a new token from the token 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 { +func (authenticator *CloudPakForDataAuthenticator) invokeRequestTokenData() error { tokenResponse, err := authenticator.requestToken() if err != nil { - authenticator.tokenData = nil + authenticator.setTokenData(nil) return err } - authenticator.tokenData, err = newCp4dTokenData(tokenResponse) - if err != nil { - authenticator.tokenData = nil + if tokenData, err := newCp4dTokenData(tokenResponse); err != nil { + authenticator.setTokenData(nil) return err + } else { + authenticator.setTokenData(tokenData) } return nil diff --git a/v5/core/cp4d_authenticator_test.go b/v5/core/cp4d_authenticator_test.go index 214a939..10289b4 100644 --- a/v5/core/cp4d_authenticator_test.go +++ b/v5/core/cp4d_authenticator_test.go @@ -192,10 +192,10 @@ func TestCp4dGetTokenSuccessPW(t *testing.T) { assert.Equal(t, cp4dUsernamePwd1, accessToken) // Force an expiration and verify we get back the second access token. - authenticator.tokenData = nil + authenticator.setTokenData(nil) accessToken, err = authenticator.getToken() assert.Nil(t, err) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) assert.Equal(t, cp4dUsernamePwd2, accessToken) } @@ -226,10 +226,10 @@ func TestCp4dGetTokenSuccessAPIKey(t *testing.T) { assert.Equal(t, cp4dUsernameApikey1, accessToken) // Force an expiration and verify we get back the second access token. - authenticator.tokenData = nil + authenticator.setTokenData(nil) accessToken, err = authenticator.getToken() assert.Nil(t, err) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) assert.Equal(t, cp4dUsernameApikey2, accessToken) } @@ -252,22 +252,22 @@ func TestCp4dGetCachedTokenSuccessPW(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingPassword(server.URL, "john", "snow", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernamePwd1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // To mock the cache, set the expiration on the existing token to be somewhere in the valid timeframe - authenticator.tokenData.Expiration = GetCurrentTime() + 9999 + authenticator.getTokenData().Expiration = GetCurrentTime() + 9999 // Subsequent fetch should still return first access token. token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernamePwd1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) } func TestCp4dGetCachedTokenAPIKey(t *testing.T) { @@ -289,22 +289,22 @@ func TestCp4dGetCachedTokenAPIKey(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingAPIKey(server.URL, "john", "King of the North", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // To mock the cache, set the expiration on the existing token to be somewhere in the valid timeframe - authenticator.tokenData.Expiration = GetCurrentTime() + 9999 + authenticator.getTokenData().Expiration = GetCurrentTime() + 9999 // Subsequent fetch should still return first access token. token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) } func TestCp4dGetTokenAuthFailure(t *testing.T) { @@ -381,17 +381,17 @@ func TestCp4dBackgroundTokenRefreshSuccess(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingAPIKey(server.URL, "john", "King of the North", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Now put the test in the "refresh window" where the token is not expired but still needs to be refreshed. - authenticator.tokenData.Expiration = GetCurrentTime() + 3600 - authenticator.tokenData.RefreshTime = GetCurrentTime() - 720 + authenticator.getTokenData().Expiration = GetCurrentTime() + 3600 + authenticator.getTokenData().RefreshTime = GetCurrentTime() - 720 // Authenticator should detect the need to refresh and request a new access token IN THE BACKGROUND when we call // getToken() again. The immediate response should be the token which was already stored, since it's not yet @@ -399,14 +399,14 @@ func TestCp4dBackgroundTokenRefreshSuccess(t *testing.T) { token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Wait for the background thread to finish. time.Sleep(5 * time.Second) token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey2, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) } func TestCp4dBackgroundTokenRefreshAuthFailure(t *testing.T) { @@ -430,17 +430,17 @@ func TestCp4dBackgroundTokenRefreshAuthFailure(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingPassword(server.URL, "john", "snow", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernamePwd1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Now put the test in the "refresh window" where the token is not expired but still needs to be refreshed. - authenticator.tokenData.Expiration = GetCurrentTime() + 3600 - authenticator.tokenData.RefreshTime = GetCurrentTime() - 720 + authenticator.getTokenData().Expiration = GetCurrentTime() + 3600 + authenticator.getTokenData().RefreshTime = GetCurrentTime() - 720 // Authenticator should detect the need to refresh and request a new access token IN THE BACKGROUND when we call // getToken() again. The immediate response should be the token which was already stored, since it's not yet @@ -448,7 +448,7 @@ func TestCp4dBackgroundTokenRefreshAuthFailure(t *testing.T) { token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernamePwd1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Wait for the background thread to finish time.Sleep(5 * time.Second) @@ -480,29 +480,29 @@ func TestCp4dBackgroundTokenRefreshIdle(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingAPIKey(server.URL, "john", "King of the North", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Now simulate the client being idle for 10 minutes into the refresh time. - authenticator.tokenData.Expiration = GetCurrentTime() + 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() + 3600 tenMinutesBeforeNow := GetCurrentTime() - 600 - authenticator.tokenData.RefreshTime = tenMinutesBeforeNow + authenticator.getTokenData().RefreshTime = tenMinutesBeforeNow // Authenticator should detect the need to refresh and request a new access token IN THE BACKGROUND when we call // getToken() again. The immediate response should be the token which was already stored, since it's not yet expired. token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // RefreshTime should have advanced by 1 minute from the current time. newRefreshTime := GetCurrentTime() + 60 - assert.Equal(t, newRefreshTime, authenticator.tokenData.RefreshTime) + assert.Equal(t, newRefreshTime, authenticator.getTokenData().RefreshTime) // In the next request, the RefreshTime should be unchanged and another thread // shouldn't be spawned to request another token once more since the first thread already spawned @@ -510,16 +510,16 @@ func TestCp4dBackgroundTokenRefreshIdle(t *testing.T) { token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) - assert.Equal(t, newRefreshTime, authenticator.tokenData.RefreshTime) + assert.NotNil(t, authenticator.getTokenData()) + assert.Equal(t, newRefreshTime, authenticator.getTokenData().RefreshTime) // Wait for the background thread to finish and verify both the RefreshTime & tokenData were updated time.Sleep(5 * time.Second) token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey2, token) - assert.NotNil(t, authenticator.tokenData) - assert.NotEqual(t, newRefreshTime, authenticator.tokenData.RefreshTime) + assert.NotNil(t, authenticator.getTokenData()) + assert.NotEqual(t, newRefreshTime, authenticator.getTokenData().RefreshTime) } func TestCp4dDisableSSL(t *testing.T) { @@ -658,16 +658,16 @@ func TestCp4dGetTokenTimeoutError(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingAPIKey(server.URL, "john", "King of the North", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernameApikey1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Force expiration and verify that we got a timeout error - authenticator.tokenData.Expiration = GetCurrentTime() - 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() - 3600 // Set the client timeout to something very low authenticator.Client.Timeout = time.Second * 2 @@ -699,16 +699,16 @@ func TestCp4dGetTokenServerError(t *testing.T) { authenticator, err := NewCloudPakForDataAuthenticatorUsingPassword(server.URL, "john", "snow", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, cp4dUsernamePwd1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Force expiration and verify that we got a server error. - authenticator.tokenData.Expiration = GetCurrentTime() - 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() - 3600 token, err = authenticator.getToken() assert.NotNil(t, err) From 9328f1611315a3b6caed995af888ef78f0a1c80c Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Tue, 23 Mar 2021 11:07:48 +0100 Subject: [PATCH 2/8] fix: make IAM authenticator token data thread safe --- v5/core/iam_authenticator.go | 42 +++++++++++++----- v5/core/iam_authenticator_test.go | 74 +++++++++++++++---------------- 2 files changed, 68 insertions(+), 48 deletions(-) diff --git a/v5/core/iam_authenticator.go b/v5/core/iam_authenticator.go index d70f1bc..4564f52 100644 --- a/v5/core/iam_authenticator.go +++ b/v5/core/iam_authenticator.go @@ -84,6 +84,9 @@ type IamAuthenticator struct { // The cached token and expiration time. tokenData *iamTokenData + + // Mutex to make the tokenData field thread safe. + tokenDataMutex sync.Mutex } var iamRequestTokenMutex sync.Mutex @@ -152,6 +155,22 @@ func (authenticator *IamAuthenticator) Authenticate(request *http.Request) error return nil } +// getTokenData returns the tokeData field from the authenticator. +func (authenticator *IamAuthenticator) getTokenData() *iamTokenData { + authenticator.tokenDataMutex.Lock() + defer authenticator.tokenDataMutex.Unlock() + + return authenticator.tokenData +} + +// setTokenData sets the given iamTokenData to the tokenData field of the authenticator. +func (authenticator *IamAuthenticator) setTokenData(tokenData *iamTokenData) { + authenticator.tokenDataMutex.Lock() + defer authenticator.tokenDataMutex.Unlock() + + authenticator.tokenData = tokenData +} + // Validate the authenticator's configuration. // // Ensures the ApiKey is valid, and the ClientId and ClientSecret pair are @@ -186,17 +205,17 @@ func (this IamAuthenticator) Validate() error { // 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 *IamAuthenticator) getToken() (string, error) { - if authenticator.tokenData == nil || !authenticator.tokenData.isTokenValid() { + if authenticator.getTokenData() == nil || !authenticator.getTokenData().isTokenValid() { // synchronously request the token err := authenticator.synchronizedRequestToken() if err != nil { return "", err } - } else if authenticator.tokenData.needsRefresh() { + } else if authenticator.getTokenData().needsRefresh() { // If refresh needed, kick off a go routine in the background to get a new token ch := make(chan error) go func() { - ch <- authenticator.getTokenData() + ch <- authenticator.invokeRequestTokenData() }() select { case err := <-ch: @@ -208,11 +227,11 @@ func (authenticator *IamAuthenticator) getToken() (string, error) { } // return an error if the access token is not valid or was not fetched - if authenticator.tokenData == nil || authenticator.tokenData.AccessToken == "" { + if authenticator.getTokenData() == nil || authenticator.getTokenData().AccessToken == "" { return "", fmt.Errorf("Error while trying to get access token") } - return authenticator.tokenData.AccessToken, nil + return authenticator.getTokenData().AccessToken, nil } // synchronizedRequestToken: synchronously checks if the current token in cache @@ -222,25 +241,26 @@ func (authenticator *IamAuthenticator) synchronizedRequestToken() error { iamRequestTokenMutex.Lock() defer iamRequestTokenMutex.Unlock() // if cached token is still valid, then just continue to use it - if authenticator.tokenData != nil && authenticator.tokenData.isTokenValid() { + if authenticator.getTokenData() != nil && authenticator.getTokenData().isTokenValid() { return nil } - return authenticator.getTokenData() + return authenticator.invokeRequestTokenData() } -// getTokenData: requests a new token from the access server and +// invokeRequestTokenData: 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 *IamAuthenticator) getTokenData() error { +func (authenticator *IamAuthenticator) invokeRequestTokenData() error { tokenResponse, err := authenticator.RequestToken() if err != nil { return err } - authenticator.tokenData, err = newIamTokenData(tokenResponse) - if err != nil { + if tokenData, err := newIamTokenData(tokenResponse); err != nil { return err + } else { + authenticator.setTokenData(tokenData) } return nil diff --git a/v5/core/iam_authenticator_test.go b/v5/core/iam_authenticator_test.go index 1f8d6f0..105b016 100644 --- a/v5/core/iam_authenticator_test.go +++ b/v5/core/iam_authenticator_test.go @@ -124,22 +124,22 @@ func TestIamGetTokenSuccess(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Force expiration and verify that we got the second access token. - authenticator.tokenData.Expiration = GetCurrentTime() - 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() - 3600 authenticator.ClientId = "mookie" authenticator.ClientSecret = "betts" _, err = authenticator.getToken() assert.Nil(t, err) - assert.NotNil(t, authenticator.tokenData) - assert.Equal(t, AccessToken2, authenticator.tokenData.AccessToken) + assert.NotNil(t, authenticator.getTokenData()) + assert.Equal(t, AccessToken2, authenticator.getTokenData().AccessToken) // Test the RequestToken() method to make sure we can get a RefreshToken. tokenResponse, err := authenticator.RequestToken() @@ -190,23 +190,23 @@ func TestIamGetTokenSuccessWithScope(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) authenticator.Scope = "scope1 scope2" // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Force expiration and verify that we got the second access token. - authenticator.tokenData.Expiration = GetCurrentTime() - 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() - 3600 authenticator.ClientId = "mookie" authenticator.ClientSecret = "betts" _, err = authenticator.getToken() assert.Nil(t, err) - assert.NotNil(t, authenticator.tokenData) - assert.Equal(t, AccessToken2, authenticator.tokenData.AccessToken) + assert.NotNil(t, authenticator.getTokenData()) + assert.Equal(t, AccessToken2, authenticator.getTokenData().AccessToken) } func TestIamGetCachedToken(t *testing.T) { firstCall := true @@ -240,19 +240,19 @@ func TestIamGetCachedToken(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Subsequent fetch should still return first access token. token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) } func TestIamBackgroundTokenRefresh(t *testing.T) { @@ -287,16 +287,16 @@ func TestIamBackgroundTokenRefresh(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Now put the test in the "refresh window" where the token is not expired but still needs to be refreshed. - authenticator.tokenData.RefreshTime = GetCurrentTime() - 720 + authenticator.getTokenData().RefreshTime = GetCurrentTime() - 720 // Authenticator should detect the need to refresh and request a new access token IN THE BACKGROUND when we call // getToken() again. The immediate response should be the token which was already stored, since it's not yet @@ -304,14 +304,14 @@ func TestIamBackgroundTokenRefresh(t *testing.T) { token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Wait for the background thread to finish time.Sleep(5 * time.Second) token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken2, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) } func TestIamBackgroundTokenRefreshFailure(t *testing.T) { @@ -338,23 +338,23 @@ func TestIamBackgroundTokenRefreshFailure(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Successfully fetch the first token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Now put the test in the "refresh window" where the token is not expired but still needs to be refreshed. - authenticator.tokenData.RefreshTime = GetCurrentTime() - 720 + authenticator.getTokenData().RefreshTime = GetCurrentTime() - 720 // Authenticator should detect the need to refresh and request a new access token IN THE BACKGROUND when we call // getToken() again. The immediate response should be the token which was already stored, since it's not yet // expired. token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Wait for the background thread to finish time.Sleep(5 * time.Second) _, err = authenticator.getToken() @@ -399,17 +399,17 @@ func TestIamBackgroundTokenRefreshIdle(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Now simulate the client being idle for 10 minutes into the refresh time tenMinutesBeforeNow := GetCurrentTime() - 600 - authenticator.tokenData.RefreshTime = tenMinutesBeforeNow + authenticator.getTokenData().RefreshTime = tenMinutesBeforeNow // Authenticator should detect the need to refresh and request a new access token IN THE BACKGROUND when we call // getToken() again. The immediate response should be the token which was already stored, since it's not yet @@ -417,11 +417,11 @@ func TestIamBackgroundTokenRefreshIdle(t *testing.T) { token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // RefreshTime should have advanced by 1 minute from the current time newRefreshTime := GetCurrentTime() + 60 - assert.Equal(t, newRefreshTime, authenticator.tokenData.RefreshTime) + assert.Equal(t, newRefreshTime, authenticator.getTokenData().RefreshTime) // In the next request, the RefreshTime should be unchanged and another thread // shouldn't be spawned to request another token once more since the first thread already spawned @@ -430,16 +430,16 @@ func TestIamBackgroundTokenRefreshIdle(t *testing.T) { assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) - assert.Equal(t, newRefreshTime, authenticator.tokenData.RefreshTime) + assert.NotNil(t, authenticator.getTokenData()) + assert.Equal(t, newRefreshTime, authenticator.getTokenData().RefreshTime) // Wait for the background thread to finish and verify both the RefreshTime & tokenData were updated time.Sleep(5 * time.Second) token, err = authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken2, token) - assert.NotNil(t, authenticator.tokenData) - assert.NotEqual(t, newRefreshTime, authenticator.tokenData.RefreshTime) + assert.NotNil(t, authenticator.getTokenData()) + assert.NotEqual(t, newRefreshTime, authenticator.getTokenData().RefreshTime) } @@ -612,16 +612,16 @@ func TestIamGetTokenTimeoutError(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) // Force expiration and verify that we got a timeout error - authenticator.tokenData.Expiration = GetCurrentTime() - 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() - 3600 // Set the client timeout to something very low authenticator.Client.Timeout = time.Second * 2 @@ -659,18 +659,18 @@ func TestIamGetTokenServerError(t *testing.T) { authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) assert.Nil(t, err) - assert.Nil(t, authenticator.tokenData) + assert.Nil(t, authenticator.getTokenData()) // Force the first fetch and verify we got the first access token. token, err := authenticator.getToken() assert.Nil(t, err) assert.Equal(t, AccessToken1, token) - assert.NotNil(t, authenticator.tokenData) + assert.NotNil(t, authenticator.getTokenData()) var expectedResponse = []byte("Gateway Timeout") // Force expiration and verify that we got a server error - authenticator.tokenData.Expiration = GetCurrentTime() - 3600 + authenticator.getTokenData().Expiration = GetCurrentTime() - 3600 token, err = authenticator.getToken() assert.NotNil(t, err) // We expect an AuthenticationError to be returned, so cast the returned error From 358a8bf52ac057c753b6315a86e8904e92d6870d Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Thu, 25 Mar 2021 12:04:16 +0100 Subject: [PATCH 3/8] fix: use mutex to prevent data race for "requestCount" in the retries test --- v5/core/base_service_retries_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/v5/core/base_service_retries_test.go b/v5/core/base_service_retries_test.go index 5178f6d..f331cd3 100644 --- a/v5/core/base_service_retries_test.go +++ b/v5/core/base_service_retries_test.go @@ -24,12 +24,15 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) +var requestCountMutex sync.Mutex + // assertResponse is a convenience function for checking various parts of a response func assertResponse(r *DetailedResponse, expectedStatusCode int, expectedContentType string) { Expect(r).ToNot(BeNil()) @@ -74,7 +77,10 @@ var _ = Describe(`Retry scenarios`, func() { defer GinkgoRecover() time.Sleep(1 * time.Second) + requestCountMutex.Lock() requestCount++ + requestCountMutex.Unlock() + w.Header().Set("Retry-After", "1") w.WriteHeader(http.StatusTooManyRequests) })) @@ -95,7 +101,9 @@ var _ = Describe(`Retry scenarios`, func() { fmt.Fprintf(GinkgoWriter, "Expected error: %s\n", err.Error()) Expect(err.Error()).To(ContainSubstring("context deadline exceeded")) Expect(resp).To(BeNil()) + requestCountMutex.Lock() Expect(requestCount).To(Equal(0)) + requestCountMutex.Unlock() }) It(`Timeout on while doing retries`, func() { service, builder := clientInit("GET", server.URL, 2, 0) From 6a55707f1728fd6f9afceafa1a921eea4fb8e028 Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Thu, 25 Mar 2021 12:19:35 +0100 Subject: [PATCH 4/8] chore: typo --- v5/core/cp4d_authenticator.go | 2 +- v5/core/iam_authenticator.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/v5/core/cp4d_authenticator.go b/v5/core/cp4d_authenticator.go index 6fd9046..7b203a8 100644 --- a/v5/core/cp4d_authenticator.go +++ b/v5/core/cp4d_authenticator.go @@ -173,7 +173,7 @@ func (authenticator *CloudPakForDataAuthenticator) Authenticate(request *http.Re return nil } -// getTokenData returns the tokeData field from the authenticator. +// getTokenData returns the tokenData field from the authenticator. func (authenticator *CloudPakForDataAuthenticator) getTokenData() *cp4dTokenData { authenticator.tokenDataMutex.Lock() defer authenticator.tokenDataMutex.Unlock() diff --git a/v5/core/iam_authenticator.go b/v5/core/iam_authenticator.go index 4564f52..6f3c629 100644 --- a/v5/core/iam_authenticator.go +++ b/v5/core/iam_authenticator.go @@ -155,7 +155,7 @@ func (authenticator *IamAuthenticator) Authenticate(request *http.Request) error return nil } -// getTokenData returns the tokeData field from the authenticator. +// getTokenData returns the tokenData field from the authenticator. func (authenticator *IamAuthenticator) getTokenData() *iamTokenData { authenticator.tokenDataMutex.Lock() defer authenticator.tokenDataMutex.Unlock() From f492206aabba290b6c81520349e4c1a858258f79 Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Thu, 25 Mar 2021 15:06:43 +0100 Subject: [PATCH 5/8] chore: fix govet copylocks error This error occures when there is a mutex in the structure and using a method with a value reviever. In this case the struct will be copied on the method call and it will create a new mutex therefore our previous lock became useless. From the go vet doc: "copylocks: check for locks erroneously passed by value" --- v5/core/cp4d_authenticator.go | 4 ++-- v5/core/iam_authenticator.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/v5/core/cp4d_authenticator.go b/v5/core/cp4d_authenticator.go index 7b203a8..e203d64 100644 --- a/v5/core/cp4d_authenticator.go +++ b/v5/core/cp4d_authenticator.go @@ -129,7 +129,7 @@ func newCloudPakForDataAuthenticatorFromMap(properties map[string]string) (*Clou } // AuthenticationType returns the authentication type for this authenticator. -func (CloudPakForDataAuthenticator) AuthenticationType() string { +func (*CloudPakForDataAuthenticator) AuthenticationType() string { return AUTHTYPE_CP4D } @@ -137,7 +137,7 @@ func (CloudPakForDataAuthenticator) AuthenticationType() string { // // Ensures the username, password, and url are not Nil. Additionally, ensures // they do not contain invalid characters. -func (authenticator CloudPakForDataAuthenticator) Validate() error { +func (authenticator *CloudPakForDataAuthenticator) Validate() error { if authenticator.Username == "" { return fmt.Errorf(ERRORMSG_PROP_MISSING, "Username") diff --git a/v5/core/iam_authenticator.go b/v5/core/iam_authenticator.go index 6f3c629..22ef5a0 100644 --- a/v5/core/iam_authenticator.go +++ b/v5/core/iam_authenticator.go @@ -135,7 +135,7 @@ func newIamAuthenticatorFromMap(properties map[string]string) (authenticator *Ia } // AuthenticationType returns the authentication type for this authenticator. -func (IamAuthenticator) AuthenticationType() string { +func (*IamAuthenticator) AuthenticationType() string { return AUTHTYPE_IAM } @@ -175,7 +175,7 @@ func (authenticator *IamAuthenticator) setTokenData(tokenData *iamTokenData) { // // Ensures the ApiKey is valid, and the ClientId and ClientSecret pair are // mutually inclusive. -func (this IamAuthenticator) Validate() error { +func (this *IamAuthenticator) Validate() error { if this.ApiKey == "" { return fmt.Errorf(ERRORMSG_PROP_MISSING, "ApiKey") } From a61fff437e2bb4f3a254e10da380dab2f6d37b32 Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Fri, 26 Mar 2021 19:19:19 +0100 Subject: [PATCH 6/8] chore: fix interface nil value check --- v5/core/base_service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/v5/core/base_service.go b/v5/core/base_service.go index 0dc3ca8..1ac7747 100644 --- a/v5/core/base_service.go +++ b/v5/core/base_service.go @@ -87,7 +87,7 @@ func NewBaseService(options *ServiceOptions) (*BaseService, error) { return nil, fmt.Errorf(ERRORMSG_PROP_INVALID, "URL") } - if options.Authenticator == nil { + if IsNil(options.Authenticator) { return nil, fmt.Errorf(ERRORMSG_NO_AUTHENTICATOR) } @@ -315,7 +315,7 @@ func (service *BaseService) Request(req *http.Request, result interface{}) (deta } // Add authentication to the outbound request. - if service.Options.Authenticator == nil { + if IsNil(service.Options.Authenticator) { err = fmt.Errorf(ERRORMSG_NO_AUTHENTICATOR) return } From 1855dbe6a567fd591ea71a95770c4c4da1e8050f Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Fri, 26 Mar 2021 19:24:42 +0100 Subject: [PATCH 7/8] chore: add test for nil interface value in the authenticator --- v5/core/base_service_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/v5/core/base_service_test.go b/v5/core/base_service_test.go index e739337..f2ae4bf 100644 --- a/v5/core/base_service_test.go +++ b/v5/core/base_service_test.go @@ -1496,6 +1496,21 @@ func TestAuthNotConfigured(t *testing.T) { assert.Nil(t, service) } +func TestAuthInterfaceNilValue(t *testing.T) { + // Verify a non-nil service is cloned correctly. + authenticator := &NoAuthAuthenticator{} + authenticator = nil + + options := &ServiceOptions{ + Authenticator: authenticator, + } + + service, err := NewBaseService(options) + assert.Nil(t, service) + assert.NotNil(t, err) + assert.Equal(t, ERRORMSG_NO_AUTHENTICATOR, err.Error()) +} + func testGetErrorMessage(t *testing.T, statusCode int, jsonString string, expectedErrorMsg string) { body := []byte(jsonString) responseMap, err := decodeAsMap(body) From 03ce576062135b4488df31ff6a8ca09c6203d27a Mon Sep 17 00:00:00 2001 From: Norbert Biczo Date: Mon, 29 Mar 2021 10:13:31 +0200 Subject: [PATCH 8/8] chore: fix lint error --- v5/core/base_service_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/v5/core/base_service_test.go b/v5/core/base_service_test.go index f2ae4bf..a891847 100644 --- a/v5/core/base_service_test.go +++ b/v5/core/base_service_test.go @@ -1497,9 +1497,7 @@ func TestAuthNotConfigured(t *testing.T) { } func TestAuthInterfaceNilValue(t *testing.T) { - // Verify a non-nil service is cloned correctly. - authenticator := &NoAuthAuthenticator{} - authenticator = nil + var authenticator *NoAuthAuthenticator = nil options := &ServiceOptions{ Authenticator: authenticator,