From 80e2eea187b1358008071113d2df71b4f92e1811 Mon Sep 17 00:00:00 2001 From: Patrick Jones Date: Thu, 17 Dec 2020 11:26:51 -0800 Subject: [PATCH] google: fixing more nits Change-Id: Ie637113fa2363a40e05f51345efbd87d22bfe54a --- .../externalaccount/basecredentials.go | 18 ++--- .../externalaccount/basecredentials_test.go | 15 ++--- .../externalaccount/filecredsource.go | 26 +++++--- .../externalaccount/filecredsource_test.go | 65 ++++++++++--------- 4 files changed, 66 insertions(+), 58 deletions(-) diff --git a/google/internal/externalaccount/basecredentials.go b/google/internal/externalaccount/basecredentials.go index 0943b39..8ae83a6 100644 --- a/google/internal/externalaccount/basecredentials.go +++ b/google/internal/externalaccount/basecredentials.go @@ -45,10 +45,9 @@ const ( ) type format struct { - // Either "text" or "json". When not provided "text" type is assumed. + // Type is either "text" or "json". When not provided "text" type is assumed. Type string `json:"type"` - // SubjectTokenFieldName is only required for JSON format. - // This would be "access_token" for azure. + // SubjectTokenFieldName is only required for JSON format. This would be "access_token" for azure. SubjectTokenFieldName string `json:"subject_token_field_name"` } @@ -88,11 +87,11 @@ type tokenSource struct { func (ts tokenSource) Token() (*oauth2.Token, error) { conf := ts.conf - typedCredSource := conf.parse() - if typedCredSource == nil { - return nil, fmt.Errorf("google: unable to parse credential source") + credSource := conf.parse() + if credSource == nil { + return nil, fmt.Errorf("oauth2/google: unable to parse credential source") } - subjectToken, err := typedCredSource.retrieveSubjectToken(conf) + subjectToken, err := credSource.retrieveSubjectToken(conf) if err != nil { return nil, err } @@ -121,7 +120,7 @@ func (ts tokenSource) Token() (*oauth2.Token, error) { TokenType: stsResp.TokenType, } if stsResp.ExpiresIn < 0 { - return nil, fmt.Errorf("google/oauth2: got invalid expiry from security token service") + return nil, fmt.Errorf("oauth2/google: got invalid expiry from security token service") } else if stsResp.ExpiresIn >= 0 { accessToken.Expiry = now().Add(time.Duration(stsResp.ExpiresIn) * time.Second) } @@ -132,6 +131,3 @@ func (ts tokenSource) Token() (*oauth2.Token, error) { return accessToken, nil } - -// 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) diff --git a/google/internal/externalaccount/basecredentials_test.go b/google/internal/externalaccount/basecredentials_test.go index 7b4145a..107ed4a 100644 --- a/google/internal/externalaccount/basecredentials_test.go +++ b/google/internal/externalaccount/basecredentials_test.go @@ -1,3 +1,7 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + package externalaccount import ( @@ -14,9 +18,8 @@ var testBaseCredSource = CredentialSource{ } var testConfig = Config{ - Audience: "32555940559.apps.googleusercontent.com", - SubjectTokenType: "urn:ietf:params:oauth:token-type:jwt", - //TokenURL: "http://localhost:8080/v1/token", + Audience: "32555940559.apps.googleusercontent.com", + SubjectTokenType: "urn:ietf:params:oauth:token-type:jwt", TokenInfoURL: "http://localhost:8080/v1/tokeninfo", ServiceAccountImpersonationURL: "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/service-gcs-admin@$PROJECT_ID.iam.gserviceaccount.com:generateAccessToken", ClientSecret: "notsosecret", @@ -38,9 +41,6 @@ var ( func TestToken_Func(t *testing.T) { targetServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - /*I'm not sure whether this testing is necessary or not. There's an argument that it should be here for completeness, - but it's also just mimicking similar testing done in sts_exchange_test.go - */ if got, want := r.URL.String(), "/"; got != want { t.Errorf("Unexpected request URL: got %v but want %v", got, want) } @@ -62,6 +62,7 @@ func TestToken_Func(t *testing.T) { w.Header().Set("Content-Type", "application/json") w.Write([]byte(baseCredsResponseBody)) })) + defer targetServer.Close() testConfig.TokenURL = targetServer.URL ourTS := tokenSource{ @@ -88,6 +89,4 @@ func TestToken_Func(t *testing.T) { 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.go b/google/internal/externalaccount/filecredsource.go index 27b3a7e..786c18f 100644 --- a/google/internal/externalaccount/filecredsource.go +++ b/google/internal/externalaccount/filecredsource.go @@ -1,6 +1,11 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + package externalaccount import ( + "bytes" "encoding/json" "errors" "fmt" @@ -15,25 +20,30 @@ type fileCredentialSource struct { func (cs fileCredentialSource) retrieveSubjectToken(c *Config) (string, error) { tokenFile, err := os.Open(cs.File) if err != nil { - return "", fmt.Errorf("Failed to open credential file %s\n", cs.File) + return "", fmt.Errorf("oauth2/google: failed to open credential file %q\n", cs.File) } - tokenBytes, _ := ioutil.ReadAll(tokenFile) - if string(tokenBytes[len(tokenBytes)-1]) == "\n" { //Deals with a possible trailing newline character - tokenBytes = tokenBytes[0 : len(tokenBytes)-1] + defer tokenFile.Close() + tokenBytes, err := ioutil.ReadAll(tokenFile) + if err != nil { + return "", fmt.Errorf("oauth2/google: failed to read credential file; %q", err) } + tokenBytes = bytes.TrimSpace(tokenBytes) var output string switch c.CredentialSource.Format.Type { case "json": jsonData := make(map[string]interface{}) - json.Unmarshal(tokenBytes, &jsonData) + err = json.Unmarshal(tokenBytes, &jsonData) + if err != nil { + return "", fmt.Errorf("oauth2/google: failed to unmarshal subject token file; %q", err) + } if val, ok := jsonData[c.CredentialSource.Format.SubjectTokenFieldName]; !ok { return "", errors.New("oauth2/google: provided subject_token_field_name not found in credentials") } else { - if token, ok := val.(string); !ok { + token, ok := val.(string) + if ok { return "", errors.New("oauth2/google: improperly formatted subject token") - } else { - output = token } + output = token } case "text": diff --git a/google/internal/externalaccount/filecredsource_test.go b/google/internal/externalaccount/filecredsource_test.go index 65a8484..f6829cb 100644 --- a/google/internal/externalaccount/filecredsource_test.go +++ b/google/internal/externalaccount/filecredsource_test.go @@ -1,3 +1,7 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + package externalaccount import ( @@ -14,38 +18,37 @@ var testFileConfig = Config{ ClientID: "rbrgnognrhongo3bi4gb9ghg9g", } -type fsTest struct { - name string - cs CredentialSource - want string -} - -var testFsUntyped = fsTest{ - name: "UntypedFileSource", - cs: CredentialSource{ - File: "./testdata/3pi_cred.txt", - }, - want: "street123", -} -var testFsTypeText = fsTest{ - name: "TextFileSource", - cs: CredentialSource{ - File: "./testdata/3pi_cred.txt", - Format: format{Type: fileTypeText}, - }, - want: "street123", -} -var testFsTypeJSON = fsTest{ - name: "JSONFileSource", - cs: CredentialSource{ - File: "./testdata/3pi_cred.json", - Format: format{Type: fileTypeJSON, SubjectTokenFieldName: "SubjToken"}, - }, - want: "321road", -} -var fileSourceTests = []fsTest{testFsUntyped, testFsTypeText, testFsTypeJSON} - func TestRetrieveFileSubjectToken(t *testing.T) { + var fileSourceTests = []struct { + name string + cs CredentialSource + want string + }{ + { + name: "UntypedFileSource", + cs: CredentialSource{ + File: "./testdata/3pi_cred.txt", + }, + want: "street123", + }, + { + name: "TextFileSource", + cs: CredentialSource{ + File: "./testdata/3pi_cred.txt", + Format: format{Type: fileTypeText}, + }, + want: "street123", + }, + { + name: "JSONFileSource", + cs: CredentialSource{ + File: "./testdata/3pi_cred.json", + Format: format{Type: fileTypeJSON, SubjectTokenFieldName: "SubjToken"}, + }, + want: "321road", + }, + } + for _, test := range fileSourceTests { tfc := testFileConfig tfc.CredentialSource = test.cs