From 8d4685e881c3f4806f971ab98f26bba64cb7b40f Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 14 Sep 2020 09:38:26 -0500 Subject: [PATCH] fix: iam/cp4d token refresh logic (#73) --- v4/core/authenticator.go | 4 +- v4/core/cp4d_authenticator.go | 2 +- v4/core/cp4d_authenticator_test.go | 104 +++++++++++++++++++++++++++++ v4/core/iam_authenticator.go | 3 +- v4/core/iam_authenticator_test.go | 79 ++++++++++++++++++++++ 5 files changed, 187 insertions(+), 5 deletions(-) diff --git a/v4/core/authenticator.go b/v4/core/authenticator.go index f199bb6..e580c65 100644 --- a/v4/core/authenticator.go +++ b/v4/core/authenticator.go @@ -35,9 +35,9 @@ func (e *AuthenticationError) Error() string { return e.Err.Error() } -func NewAuthenticationError(response *DetailedResponse, err error) (*AuthenticationError) { +func NewAuthenticationError(response *DetailedResponse, err error) *AuthenticationError { return &AuthenticationError{ Response: response, - Err: err, + Err: err, } } diff --git a/v4/core/cp4d_authenticator.go b/v4/core/cp4d_authenticator.go index cbc5f53..b6f3292 100644 --- a/v4/core/cp4d_authenticator.go +++ b/v4/core/cp4d_authenticator.go @@ -335,7 +335,7 @@ func (this *cp4dTokenData) needsRefresh() bool { // Advance refresh by one minute if this.RefreshTime >= 0 && GetCurrentTime() > this.RefreshTime { - this.RefreshTime += 60 + this.RefreshTime = GetCurrentTime() + 60 return true } diff --git a/v4/core/cp4d_authenticator_test.go b/v4/core/cp4d_authenticator_test.go index 43ebdf5..1a27895 100644 --- a/v4/core/cp4d_authenticator_test.go +++ b/v4/core/cp4d_authenticator_test.go @@ -372,6 +372,110 @@ func TestCp4dBackgroundTokenRefreshFailure(t *testing.T) { assert.False(t, ok) } +func TestCp4dBackgroundTokenRefreshIdle(t *testing.T) { + firstCall := true + accessToken1 := "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VybmFtZSI6ImhlbGxvIiwicm9sZSI6InVzZXIiLCJwZXJtaXNzaW9ucyI6WyJhZG1pbmlzdHJhdG9yIiwiZGVwbG95bWVudF9hZG1pbiJdLCJzdWIiOiJoZWxsbyIsImlzcyI6IkpvaG4iLCJhdWQiOiJEU1giLCJ1aWQiOiI5OTkiLCJpYXQiOjE1NjAyNzcwNTEsImV4cCI6MTU2MDI4MTgxOSwianRpIjoiMDRkMjBiMjUtZWUyZC00MDBmLTg2MjMtOGNkODA3MGI1NDY4In0.cIodB4I6CCcX8vfIImz7Cytux3GpWyObt9Gkur5g1QI" + accessToken2 := "3yJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VybmFtZSI6ImhlbGxvIiwicm9sZSI6InVzZXIiLCJwZXJtaXNzaW9ucyI6WyJhZG1pbmlzdHJhdG9yIiwiZGVwbG95bWVudF9hZG1pbiJdLCJzdWIiOiJoZWxsbyIsImlzcyI6IkpvaG4iLCJhdWQiOiJEU1giLCJ1aWQiOiI5OTkiLCJpYXQiOjE1NjAyNzcwNTEsImV4cCI6MTU2MDI4MTgxOSwianRpIjoiMDRkMjBiMjUtZWUyZC00MDBmLTg2MjMtOGNkODA3MGI1NDY4In0.cIodB4I6CCcX8vfIImz7Cytux3GpWyObt9Gkur5g1QI" + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if firstCall { + fmt.Fprintf(w, `{ + "username":"hello", + "role":"user", + "permissions":[ + "administrator", + "deployment_admin" + ], + "sub":"hello", + "iss":"John", + "aud":"DSX", + "uid":"999", + "accessToken": %q, + "_messageCode_":"success", + "message":"success" + }`, accessToken1) + firstCall = false + username, password, ok := r.BasicAuth() + assert.Equal(t, true, ok) + assert.Equal(t, "john", username) + assert.Equal(t, "snow", password) + } else { + fmt.Fprintf(w, `{ + "username": "admin", + "role": "Admin", + "permissions": [ + "administrator", + "manage_catalog", + "access_catalog", + "manage_policies", + "access_policies", + "virtualize_transform", + "can_provision", + "deployment_admin" + ], + "sub": "admin", + "iss": "test", + "aud": "DSX", + "uid": "999", + "accessToken": %q, + "_messageCode_": "success", + "message": "success" + }`, accessToken2) + username, password, ok := r.BasicAuth() + assert.Equal(t, true, ok) + assert.Equal(t, "john", username) + assert.Equal(t, "snow", password) + } + })) + defer server.Close() + + authenticator, err := NewCloudPakForDataAuthenticator(server.URL, "john", "snow", false, nil) + assert.Nil(t, err) + assert.Nil(t, authenticator.tokenData) + + // // 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) + + // Now simulate the client being idle for 10 minutes into the refresh time + authenticator.tokenData.Expiration = GetCurrentTime() + 3600 + tenMinutesBeforeNow := GetCurrentTime() - 600 + authenticator.tokenData.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, accessToken1, + token) + assert.NotNil(t, authenticator.tokenData) + // RefreshTime should have advanced by 1 minute from the current time + newRefreshTime := GetCurrentTime() + 60 + assert.Equal(t, newRefreshTime, authenticator.tokenData.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 + // a goroutine & refreshed the token. + token, err = authenticator.getToken() + assert.Nil(t, err) + assert.Equal(t, accessToken1, + token) + assert.NotNil(t, authenticator.tokenData) + assert.Equal(t, newRefreshTime, authenticator.tokenData.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) + +} + func TestCp4dDisableSSL(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) diff --git a/v4/core/iam_authenticator.go b/v4/core/iam_authenticator.go index f959c6b..f5b90c9 100644 --- a/v4/core/iam_authenticator.go +++ b/v4/core/iam_authenticator.go @@ -299,7 +299,6 @@ func (authenticator *IamAuthenticator) requestToken() (*iamTokenServerResponse, Headers: resp.Header, RawResult: buff.Bytes(), } - return nil, NewAuthenticationError(detailedResponse, fmt.Errorf(buff.String())) } @@ -362,7 +361,7 @@ func (this *iamTokenData) needsRefresh() bool { // Advance refresh by one minute if this.RefreshTime >= 0 && GetCurrentTime() > this.RefreshTime { - this.RefreshTime += 60 + this.RefreshTime = GetCurrentTime() + 60 return true } diff --git a/v4/core/iam_authenticator_test.go b/v4/core/iam_authenticator_test.go index 1700eb8..7198f3b 100644 --- a/v4/core/iam_authenticator_test.go +++ b/v4/core/iam_authenticator_test.go @@ -293,6 +293,85 @@ func TestIamBackgroundTokenRefreshFailure(t *testing.T) { } +func TestIamBackgroundTokenRefreshIdle(t *testing.T) { + firstCall := true + accessToken1 := "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VybmFtZSI6ImhlbGxvIiwicm9sZSI6InVzZXIiLCJwZXJtaXNzaW9ucyI6WyJhZG1pbmlzdHJhdG9yIiwiZGVwbG95bWVudF9hZG1pbiJdLCJzdWIiOiJoZWxsbyIsImlzcyI6IkpvaG4iLCJhdWQiOiJEU1giLCJ1aWQiOiI5OTkiLCJpYXQiOjE1NjAyNzcwNTEsImV4cCI6MTU2MDI4MTgxOSwianRpIjoiMDRkMjBiMjUtZWUyZC00MDBmLTg2MjMtOGNkODA3MGI1NDY4In0.cIodB4I6CCcX8vfIImz7Cytux3GpWyObt9Gkur5g1QI" + accessToken2 := "3yJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VybmFtZSI6ImhlbGxvIiwicm9sZSI6InVzZXIiLCJwZXJtaXNzaW9ucyI6WyJhZG1pbmlzdHJhdG9yIiwiZGVwbG95bWVudF9hZG1pbiJdLCJzdWIiOiJoZWxsbyIsImlzcyI6IkpvaG4iLCJhdWQiOiJEU1giLCJ1aWQiOiI5OTkiLCJpYXQiOjE1NjAyNzcwNTEsImV4cCI6MTU2MDI4MTgxOSwianRpIjoiMDRkMjBiMjUtZWUyZC00MDBmLTg2MjMtOGNkODA3MGI1NDY4In0.cIodB4I6CCcX8vfIImz7Cytux3GpWyObt9Gkur5g1QI" + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // CurrentTime + 1 hour + expiration := GetCurrentTime() + 3600 + w.WriteHeader(http.StatusOK) + if firstCall { + fmt.Fprintf(w, `{ + "access_token": %q, + "token_type": "Bearer", + "expires_in": 3600, + "expiration": %d, + "refresh_token": "jy4gl91BQ" + }`, accessToken1, expiration) + firstCall = false + _, _, ok := r.BasicAuth() + assert.Equal(t, ok, false) + } else { + fmt.Fprintf(w, `{ + "access_token": %q, + "token_type": "Bearer", + "expires_in": 3600, + "expiration": %d, + "refresh_token": "jy4gl91BQ" + }`, accessToken2, expiration) + _, _, ok := r.BasicAuth() + assert.Equal(t, ok, false) + } + })) + defer server.Close() + + authenticator, err := NewIamAuthenticator("bogus-apikey", server.URL, "", "", false, nil) + assert.Nil(t, err) + assert.Nil(t, authenticator.tokenData) + + // 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) + + // Now simulate the client being idle for 10 minutes into the refresh time + tenMinutesBeforeNow := GetCurrentTime() - 600 + authenticator.tokenData.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, accessToken1, + token) + assert.NotNil(t, authenticator.tokenData) + // RefreshTime should have advanced by 1 minute from the current time + newRefreshTime := GetCurrentTime() + 60 + assert.Equal(t, newRefreshTime, authenticator.tokenData.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 + // a goroutine & refreshed the token. + token, err = authenticator.getToken() + assert.Nil(t, err) + assert.Equal(t, accessToken1, + token) + assert.NotNil(t, authenticator.tokenData) + assert.Equal(t, newRefreshTime, authenticator.tokenData.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) + +} + func TestIamClientIdAndSecret(t *testing.T) { expiration := GetCurrentTime() + 3600 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {