From 4c1e0860ce989c16a62ff4a917ae9d3da209dc41 Mon Sep 17 00:00:00 2001 From: Patrick Jones Date: Mon, 14 Dec 2020 12:51:43 -0800 Subject: [PATCH] google: fix nomenclature feedback and changed time.Now implementation to allow for testing for time in unit tests. The unit tests will not pass at the moment; awaiting feedback on whether to use a test credential source or rely on a separate one. Change-Id: I20605fa161911f325ab41fc345436d08ed17ed5e --- .../externalaccount/basecredentials.go | 29 ++++++----------- .../externalaccount/basecredentials_test.go | 31 ++++++++++++++----- .../externalaccount/filecredsource_test.go | 2 +- 3 files changed, 33 insertions(+), 29 deletions(-) diff --git a/google/internal/externalaccount/basecredentials.go b/google/internal/externalaccount/basecredentials.go index 62ceffb..2ef1dc3 100644 --- a/google/internal/externalaccount/basecredentials.go +++ b/google/internal/externalaccount/basecredentials.go @@ -12,6 +12,9 @@ import ( "time" ) +// now aliases time.Now for testing +var now = time.Now + // Config stores the configuration for fetching tokens with external credentials. type Config struct { Audience string @@ -64,15 +67,9 @@ type CredentialSource struct { } // instance determines the type of CredentialSource needed -func (cs CredentialSource) instance() baseCredentialSource { - if cs.EnvironmentID == "awsX" { - return nil - } else if cs.File == "internalTestingFile" { - return testCredentialSource{} - } else if cs.File != "" { - return fileCredentialSource{File: cs.File} - } else if cs.URL != "" { - return nil +func (c *Config) parse() baseCredentialSource { + if c.CredentialSource.File != "" { + return fileCredentialSource{File: c.CredentialSource.File} } return nil } @@ -91,7 +88,7 @@ type tokenSource struct { func (ts tokenSource) Token() (*oauth2.Token, error) { conf := ts.conf - subjectToken, err := conf.CredentialSource.instance().retrieveSubjectToken(conf) + subjectToken, err := conf.parse().retrieveSubjectToken(conf) if err != nil { return nil, err } @@ -122,8 +119,9 @@ func (ts tokenSource) Token() (*oauth2.Token, error) { if stsResp.ExpiresIn < 0 { return nil, fmt.Errorf("google/oauth2: got invalid expiry from security token service") } else if stsResp.ExpiresIn > 0 { - accessToken.Expiry = time.Now().Add(time.Duration(stsResp.ExpiresIn) * time.Second) + accessToken.Expiry = now().Add(time.Duration(stsResp.ExpiresIn) * time.Second) } + // TODO: For reviewers- what should I do if ExpiresIn is equal to 0? Would that correspond to a one-use token, or something else? if stsResp.RefreshToken != "" { accessToken.RefreshToken = stsResp.RefreshToken @@ -134,12 +132,3 @@ func (ts tokenSource) Token() (*oauth2.Token, error) { // NOTE: this method doesn't exist yet. It is being investigated to add this method to oauth2.TokenSource. //func (ts tokenSource) TokenInfo() (*oauth2.TokenInfo, error) - -// testCredentialSource is only used for testing, but must be defined here in order to avoid undefined errors when testing. -type testCredentialSource struct { - File string -} - -func (cs testCredentialSource) retrieveSubjectToken(c *Config) (string, error) { - return "Sample.Subject.Token", nil -} diff --git a/google/internal/externalaccount/basecredentials_test.go b/google/internal/externalaccount/basecredentials_test.go index 7d97c00..194b24a 100644 --- a/google/internal/externalaccount/basecredentials_test.go +++ b/google/internal/externalaccount/basecredentials_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" ) var testBaseCredSource = CredentialSource{ @@ -24,10 +25,15 @@ var testConfig = Config{ Scopes: []string{"https://www.googleapis.com/auth/devstorage.full_control"}, } -var baseCredsRequestBody = "audience=32555940559.apps.googleusercontent.com&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&options=null&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=Sample.Subject.Token&subject_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Ajwt" -var baseCredsResponseBody = `{"access_token":"Sample.Access.Token","issued_token_type":"urn:ietf:params:oauth:token-type:access_token","token_type":"Bearer","expires_in":3600,"scope":"https://www.googleapis.com/auth/cloud-platform"}` - -var correctAT = "Sample.Access.Token" +var ( + baseCredsRequestBody = "audience=32555940559.apps.googleusercontent.com&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&options=null&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=Sample.Subject.Token&subject_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Ajwt" + baseCredsResponseBody = `{"access_token":"Sample.Access.Token","issued_token_type":"urn:ietf:params:oauth:token-type:access_token","token_type":"Bearer","expires_in":3600,"scope":"https://www.googleapis.com/auth/cloud-platform"}` + correctAT = "Sample.Access.Token" + expiry int64 = 234852 +) +var ( + testNow = func() time.Time { return time.Unix(expiry, 0) } +) func TestToken_Func(t *testing.T) { @@ -63,16 +69,25 @@ func TestToken_Func(t *testing.T) { conf: &testConfig, } + oldNow := now + defer func() { now = oldNow }() + now = testNow + tok, err := ourTS.Token() if err != nil { t.Errorf("Unexpected error: %e", err) } - if tok.AccessToken != correctAT { - t.Errorf("Unexpected access token: got %v, but wanted %v", tok.AccessToken, correctAT) + if got, want := tok.AccessToken, correctAT; got != want { + t.Errorf("Unexpected access token: got %v, but wanted %v", got, want) } - if tok.TokenType != "Bearer" { - t.Errorf("Unexpected TokenType: got %v, but wanted \"Bearer\"", tok.TokenType) + if got, want := tok.TokenType, "Bearer"; got != want { + t.Errorf("Unexpected TokenType: got %v, but wanted %v", got, want) } + + if got, want := tok.Expiry, now().Add(time.Duration(3600)*time.Second); got != want { + t.Errorf("Unexpected Expiry: got %v, but wanted %v", got, want) + } + //We don't check the correct expiry here because that's dependent on the current time. } diff --git a/google/internal/externalaccount/filecredsource_test.go b/google/internal/externalaccount/filecredsource_test.go index dc46427..4218d70 100644 --- a/google/internal/externalaccount/filecredsource_test.go +++ b/google/internal/externalaccount/filecredsource_test.go @@ -50,7 +50,7 @@ func TestRetrieveFileSubjectToken(t *testing.T) { tfc := testFileConfig tfc.CredentialSource = test.cs - out, err := test.cs.instance().retrieveSubjectToken(&tfc) + out, err := tfc.parse().retrieveSubjectToken(&tfc) if err != nil { t.Errorf("Method retrieveSubjectToken for type fileCredentialSource in test %v failed; %e", test.name, err) }