more tests, refactoring tests, not submitting userProject when unneeded

Author: Ryan Kohler

google/internal/externalaccount/basecredentials.go

@@ -127,7 +127,7 @@ func (c *Config) tokenSource(ctx context.Context, tokenURLValidPats []*regexp.Re
 	if c.WorkforcePoolUserProject != "" {
 		valid := validateWorkforceAudience(c.Audience)
 		if !valid {
-			return nil, fmt.Errorf("oauth2/google: invalid Workforce Pool Audience provided while constructing tokenSource")
+			return nil, fmt.Errorf("oauth2/google: workforce_pool_user_project should not be set for non-workforce pool credentials")
 		}
 	}
 
@@ -241,7 +241,9 @@ func (ts tokenSource) Token() (*oauth2.Token, error) {
 		ClientSecret: conf.ClientSecret,
 	}
 	var options map[string]interface{}
-	if conf.WorkforcePoolUserProject != "" {
+	// Do not pass workforce_pool_user_project when client authentication is used.
+	// The client ID is sufficient for determining the user project.
+	if conf.WorkforcePoolUserProject != "" && conf.ClientID == "" {
 		options = map[string]interface{}{
 			"userProject": conf.WorkforcePoolUserProject,
 		}

google/internal/externalaccount/basecredentials_test.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"golang.org/x/oauth2"
 )
 
 const (
@@ -35,117 +37,178 @@ var testConfig = Config{
 }
 
 var (
-	baseCredsRequestBody           = "audience=32555940559.apps.googleusercontent.com&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=street123&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"}`
-	workforcePoolRequestBody       = "audience=%2F%2Fiam.googleapis.com%2Flocations%2Feu%2FworkforcePools%2Fpool-id%2Fproviders%2Fprovider-id&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&options=%7B%22userProject%22%3A%22myProject%22%7D&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=street123&subject_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Ajwt"
-	correctAT                      = "Sample.Access.Token"
-	expiry                   int64 = 234852
+	baseCredsRequestBody                          = "audience=32555940559.apps.googleusercontent.com&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=street123&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"}`
+	workforcePoolRequestBodyWithClientId          = "audience=%2F%2Fiam.googleapis.com%2Flocations%2Feu%2FworkforcePools%2Fpool-id%2Fproviders%2Fprovider-id&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=street123&subject_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Ajwt"
+	workforcePoolRequestBodyWithoutClientId       = "audience=%2F%2Fiam.googleapis.com%2Flocations%2Feu%2FworkforcePools%2Fpool-id%2Fproviders%2Fprovider-id&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&options=%7B%22userProject%22%3A%22myProject%22%7D&requested_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&scope=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control&subject_token=street123&subject_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Ajwt"
+	correctAT                                     = "Sample.Access.Token"
+	expiry                                  int64 = 234852
 )
 var (
 	testNow = func() time.Time { return time.Unix(expiry, 0) }
 )
 
-func TestToken(t *testing.T) {
-	targetServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if got, want := r.URL.String(), "/"; got != want {
+type testExchangeTokenServer struct {
+	url           string
+	authorization string
+	contentType   string
+	body          string
+	response      string
+}
+
+func run(t *testing.T, config *Config, tets *testExchangeTokenServer) (*oauth2.Token, error) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if got, want := r.URL.String(), tets.url; got != want {
 			t.Errorf("URL.String(): got %v but want %v", got, want)
 		}
 		headerAuth := r.Header.Get("Authorization")
-		if got, want := headerAuth, "Basic cmJyZ25vZ25yaG9uZ28zYmk0Z2I5Z2hnOWc6bm90c29zZWNyZXQ="; got != want {
+		if got, want := headerAuth, tets.authorization; got != want {
 			t.Errorf("got %v but want %v", got, want)
 		}
 		headerContentType := r.Header.Get("Content-Type")
-		if got, want := headerContentType, "application/x-www-form-urlencoded"; got != want {
+		if got, want := headerContentType, tets.contentType; got != want {
 			t.Errorf("got %v but want %v", got, want)
 		}
 		body, err := ioutil.ReadAll(r.Body)
 		if err != nil {
 			t.Fatalf("Failed reading request body: %s.", err)
 		}
-		if got, want := string(body), baseCredsRequestBody; got != want {
+		if got, want := string(body), tets.body; got != want {
 			t.Errorf("Unexpected exchange payload: got %v but want %v", got, want)
 		}
 		w.Header().Set("Content-Type", "application/json")
-		w.Write([]byte(baseCredsResponseBody))
+		w.Write([]byte(tets.response))
 	}))
-	defer targetServer.Close()
-
-	testConfig.TokenURL = targetServer.URL
-	ourTS := tokenSource{
-		ctx:  context.Background(),
-		conf: &testConfig,
-	}
+	defer server.Close()
+	config.TokenURL = server.URL
 
 	oldNow := now
 	defer func() { now = oldNow }()
 	now = testNow
 
-	tok, err := ourTS.Token()
-	if err != nil {
-		t.Fatalf("Unexpected error: %e", err)
+	ts := tokenSource{
+		ctx:  context.Background(),
+		conf: config,
 	}
+
+	return ts.Token()
+}
+
+func validateToken(t *testing.T, tok *oauth2.Token) {
 	if got, want := tok.AccessToken, correctAT; got != want {
 		t.Errorf("Unexpected access token: got %v, but wanted %v", got, want)
 	}
 	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 {
+	if got, want := tok.Expiry, testNow().Add(time.Duration(3600)*time.Second); got != want {
 		t.Errorf("Unexpected Expiry: got %v, but wanted %v", got, want)
 	}
 }
 
-func TestWorkforcePoolToken(t *testing.T) {
-	targetServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if got, want := r.URL.String(), "/"; got != want {
-			t.Errorf("URL.String(): got %v but want %v", got, want)
-		}
-		headerAuth := r.Header.Get("Authorization")
-		if got, want := headerAuth, "Basic cmJyZ25vZ25yaG9uZ28zYmk0Z2I5Z2hnOWc6bm90c29zZWNyZXQ="; got != want {
-			t.Errorf("got %v but want %v", got, want)
-		}
-		headerContentType := r.Header.Get("Content-Type")
-		if got, want := headerContentType, "application/x-www-form-urlencoded"; got != want {
-			t.Errorf("got %v but want %v", got, want)
-		}
-		body, err := ioutil.ReadAll(r.Body)
-		if err != nil {
-			t.Fatalf("Failed reading request body: %s.", err)
-		}
-		if got, want := string(body), workforcePoolRequestBody; got != want {
-			t.Errorf("Unexpected exchange payload: got %v but want %v", got, want)
-		}
-		w.Header().Set("Content-Type", "application/json")
-		w.Write([]byte(baseCredsResponseBody))
-	}))
-	defer targetServer.Close()
+func TestToken(t *testing.T) {
+	config := Config{
+		Audience:         "32555940559.apps.googleusercontent.com",
+		SubjectTokenType: "urn:ietf:params:oauth:token-type:jwt",
+		TokenInfoURL:     "http://localhost:8080/v1/tokeninfo",
+		ClientSecret:     "notsosecret",
+		ClientID:         "rbrgnognrhongo3bi4gb9ghg9g",
+		CredentialSource: testBaseCredSource,
+		Scopes:           []string{"https://www.googleapis.com/auth/devstorage.full_control"},
+	}
 
-	testConfig.TokenURL = targetServer.URL
-	testConfig.WorkforcePoolUserProject = "myProject"
-	testConfig.Audience = "//iam.googleapis.com/locations/eu/workforcePools/pool-id/providers/provider-id"
-	ourTS := tokenSource{
-		ctx:  context.Background(),
-		conf: &testConfig,
+	server := testExchangeTokenServer{
+		url:           "/",
+		authorization: "Basic cmJyZ25vZ25yaG9uZ28zYmk0Z2I5Z2hnOWc6bm90c29zZWNyZXQ=",
+		contentType:   "application/x-www-form-urlencoded",
+		body:          baseCredsRequestBody,
+		response:      baseCredsResponseBody,
 	}
 
-	oldNow := now
-	defer func() { now = oldNow }()
-	now = testNow
+	tok, err := run(t, &config, &server)
 
-	tok, err := ourTS.Token()
 	if err != nil {
 		t.Fatalf("Unexpected error: %e", err)
 	}
-	if got, want := tok.AccessToken, correctAT; got != want {
-		t.Errorf("Unexpected access token: got %v, but wanted %v", got, want)
+	validateToken(t, tok)
+}
+
+func TestWorkforcePoolTokenWithClientID(t *testing.T) {
+	config := Config{
+		Audience:                 "//iam.googleapis.com/locations/eu/workforcePools/pool-id/providers/provider-id",
+		SubjectTokenType:         "urn:ietf:params:oauth:token-type:jwt",
+		TokenInfoURL:             "http://localhost:8080/v1/tokeninfo",
+		ClientSecret:             "notsosecret",
+		ClientID:                 "rbrgnognrhongo3bi4gb9ghg9g",
+		CredentialSource:         testBaseCredSource,
+		Scopes:                   []string{"https://www.googleapis.com/auth/devstorage.full_control"},
+		WorkforcePoolUserProject: "myProject",
 	}
-	if got, want := tok.TokenType, "Bearer"; got != want {
-		t.Errorf("Unexpected TokenType: got %v, but wanted %v", got, want)
+
+	server := testExchangeTokenServer{
+		url:           "/",
+		authorization: "Basic cmJyZ25vZ25yaG9uZ28zYmk0Z2I5Z2hnOWc6bm90c29zZWNyZXQ=",
+		contentType:   "application/x-www-form-urlencoded",
+		body:          workforcePoolRequestBodyWithClientId,
+		response:      baseCredsResponseBody,
 	}
 
-	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)
+	tok, err := run(t, &config, &server)
+
+	if err != nil {
+		t.Fatalf("Unexpected error: %e", err)
+	}
+	validateToken(t, tok)
+}
+
+func TestWorkforcePoolTokenWithoutClientID(t *testing.T) {
+	config := Config{
+		Audience:                 "//iam.googleapis.com/locations/eu/workforcePools/pool-id/providers/provider-id",
+		SubjectTokenType:         "urn:ietf:params:oauth:token-type:jwt",
+		TokenInfoURL:             "http://localhost:8080/v1/tokeninfo",
+		ClientSecret:             "notsosecret",
+		CredentialSource:         testBaseCredSource,
+		Scopes:                   []string{"https://www.googleapis.com/auth/devstorage.full_control"},
+		WorkforcePoolUserProject: "myProject",
+	}
+
+	server := testExchangeTokenServer{
+		url:           "/",
+		authorization: "",
+		contentType:   "application/x-www-form-urlencoded",
+		body:          workforcePoolRequestBodyWithoutClientId,
+		response:      baseCredsResponseBody,
+	}
+
+	tok, err := run(t, &config, &server)
+
+	if err != nil {
+		t.Fatalf("Unexpected error: %e", err)
+	}
+	validateToken(t, tok)
+}
+
+func TestNonworkforceWithWorkforcePoolUserProject(t *testing.T) {
+	config := Config{
+		Audience:                 "32555940559.apps.googleusercontent.com",
+		SubjectTokenType:         "urn:ietf:params:oauth:token-type:jwt",
+		TokenInfoURL:             "http://localhost:8080/v1/tokeninfo",
+		TokenURL:                 "https://sts.googleapis.com",
+		ClientSecret:             "notsosecret",
+		ClientID:                 "rbrgnognrhongo3bi4gb9ghg9g",
+		CredentialSource:         testBaseCredSource,
+		Scopes:                   []string{"https://www.googleapis.com/auth/devstorage.full_control"},
+		WorkforcePoolUserProject: "myProject",
+	}
+
+	_, err := config.TokenSource(context.Background())
+
+	if err == nil {
+		t.Fatalf("Expected error but found none")
+	}
+	if got, want := err.Error(), "oauth2/google: workforce_pool_user_project should not be set for non-workforce pool credentials"; got != want {
+		t.Errorf("Incorrect error received.\nExpected: %s\nRecieved: %s", want, got)
 	}
 }