google: fixing more nits

Change-Id: Ie637113fa2363a40e05f51345efbd87d22bfe54a

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.
+	// SubjectTokenFieldName is only required for JSON format.  This would be "access_token" for azure.
-	// 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()
+	credSource := conf.parse()
-	if typedCredSource == nil {
+	if credSource == nil {
-		return nil, fmt.Errorf("google: unable to parse credential source")
+		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)

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",
+	Audience:                       "32555940559.apps.googleusercontent.com",
-	SubjectTokenType: "urn:ietf:params:oauth:token-type:jwt",
+	SubjectTokenType:               "urn:ietf:params:oauth:token-type:jwt",
-	//TokenURL: "http://localhost:8080/v1/token",
 	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.
-
 }

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)
+	defer tokenFile.Close()
-	if string(tokenBytes[len(tokenBytes)-1]) == "\n" { //Deals with a possible trailing newline character
+	tokenBytes, err := ioutil.ReadAll(tokenFile)
-		tokenBytes = tokenBytes[0 : len(tokenBytes)-1]
+	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":

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