chore(auth): cleanup some small todos (#8628)

fixup various small todos, still more to come
googleapis · Sep 29, 2023 · 852a230 · 852a230
1 parent dd0b039
commit 852a230
Show file tree

Hide file tree

Showing 8 changed files with 94 additions and 168 deletions.
diff --git a/auth/detect/compute.go b/auth/detect/compute.go
@@ -55,8 +55,6 @@ type metadataTokenResp struct {
 }
 
 func (cs computeProvider) Token(ctx context.Context) (*auth.Token, error) {
-	// TODO(codyoss): account may need to be configurable if we have a constructor for
-	// the provider.
 	tokenURI, err := url.Parse(computeTokenURI)
 	if err != nil {
 		return nil, err

diff --git a/auth/detect/example_test.go b/auth/detect/example_test.go
@@ -19,6 +19,7 @@ import (
 	"os"
 
 	"cloud.google.com/go/auth/detect"
+	"cloud.google.com/go/auth/httptransport"
 )
 
 func ExampleDefaultCredentials() {
@@ -28,16 +29,13 @@ func ExampleDefaultCredentials() {
 	if err != nil {
 		log.Fatal(err)
 	}
-	_ = creds
-	// TODO(codyoss):fixup after more code is added
-
-	// client, err := httptransport.NewClient(&httptransport.Options{
-	// 	TokenProvider: creds,
-	// })
-	// if err != nil {
-	// 	log.Fatal(err)
-	// }
-	// client.Get("...")
+	client, err := httptransport.NewClient(&httptransport.Options{
+		TokenProvider: creds,
+	})
+	if err != nil {
+		log.Fatal(err)
+	}
+	client.Get("...")
 }
 
 func ExampleDefaultCredentials_withFilepath() {
@@ -56,16 +54,13 @@ func ExampleDefaultCredentials_withFilepath() {
 	if err != nil {
 		log.Fatal(err)
 	}
-	_ = creds
-	// TODO(codyoss):fixup after more code is added
-
-	// client, err := httptransport.NewClient(&httptransport.Options{
-	// 	TokenProvider: creds,
-	// })
-	// if err != nil {
-	// 	log.Fatal(err)
-	// }
-	// client.Get("...")
+	client, err := httptransport.NewClient(&httptransport.Options{
+		TokenProvider: creds,
+	})
+	if err != nil {
+		log.Fatal(err)
+	}
+	client.Get("...")
 }
 
 func ExampleDefaultCredentials_withJSON() {
@@ -80,14 +75,11 @@ func ExampleDefaultCredentials_withJSON() {
 	if err != nil {
 		log.Fatal(err)
 	}
-	_ = creds
-	// TODO(codyoss):fixup after more code is added
-
-	// client, err := httptransport.NewClient(&httptransport.Options{
-	// 	TokenProvider: creds,
-	// })
-	// if err != nil {
-	// 	log.Fatal(err)
-	// }
-	// client.Get("...")
+	client, err := httptransport.NewClient(&httptransport.Options{
+		TokenProvider: creds,
+	})
+	if err != nil {
+		log.Fatal(err)
+	}
+	client.Get("...")
 }
diff --git a/auth/detect/internal/externalaccount/aws_provider.go b/auth/detect/internal/externalaccount/aws_provider.go
@@ -234,7 +234,6 @@ func (cs *awsSubjectProvider) getRegion(ctx context.Context, headers map[string]
 	// Only the us-east-2 part should be used.
 	bodyLen := len(respBody)
 	if bodyLen == 0 {
-		// TODO(codyoss): this was the old behaviour, but maybe this should be an error
 		return "", nil
 	}
 	return string(respBody[:bodyLen-1]), nil

diff --git a/auth/example_test.go b/auth/example_test.go
@@ -15,7 +15,10 @@
 package auth_test
 
 import (
+	"log"
+
 	"cloud.google.com/go/auth"
+	"cloud.google.com/go/auth/httptransport"
 )
 
 func ExampleNew2LOTokenProvider() {
@@ -47,15 +50,14 @@ func ExampleNew2LOTokenProvider() {
 
 	tp, err := auth.New2LOTokenProvider(opts)
 	if err != nil {
-		// handler error
+		log.Fatal(err)
+	}
+	client, err := httptransport.NewClient(&httptransport.Options{
+		TokenProvider: tp,
+	})
+	if err != nil {
+		log.Fatal(err)
 	}
-	// TODO(codyoss): Fixup once more code is merged
-	// client, err := httptransport.NewClient(&httptransport.Options{
-	// 	TokenProvider: tp,
-	// })
-	// if err != nil {
-	// 	log.Fatal(err)
-	// }
-	// client.Get("...")
+	client.Get("...")
 	_ = tp
 }
diff --git a/auth/impersonate/integration_test.go b/auth/impersonate/integration_test.go
@@ -28,6 +28,7 @@ import (
 	"cloud.google.com/go/auth/impersonate"
 	"cloud.google.com/go/auth/internal/testutil"
 	"cloud.google.com/go/auth/internal/testutil/testgcs"
+	"google.golang.org/api/idtoken"
 )
 
 const (
@@ -117,63 +118,61 @@ func TestCredentialsTokenSourceIntegration(t *testing.T) {
 	}
 }
 
-// TODO(codyoss): uncomment after adding idtoken package
-
-// func TestIDTokenSourceIntegration(t *testing.T) {
-// 	testutil.IntegrationTestCheck(t)
+func TestIDTokenSourceIntegration(t *testing.T) {
+	testutil.IntegrationTestCheck(t)
 
-// 	ctx := context.Background()
-// 	tests := []struct {
-// 		name        string
-// 		baseKeyFile string
-// 		delegates   []string
-// 	}{
-// 		{
-// 			name:        "SA -> SA",
-// 			baseKeyFile: readerKeyFile,
-// 		},
-// 		{
-// 			name:        "SA -> Delegate -> SA",
-// 			baseKeyFile: baseKeyFile,
-// 			delegates:   []string{readerEmail},
-// 		},
-// 	}
+	ctx := context.Background()
+	tests := []struct {
+		name        string
+		baseKeyFile string
+		delegates   []string
+	}{
+		{
+			name:        "SA -> SA",
+			baseKeyFile: readerKeyFile,
+		},
+		{
+			name:        "SA -> Delegate -> SA",
+			baseKeyFile: baseKeyFile,
+			delegates:   []string{readerEmail},
+		},
+	}
 
-// 	for _, tt := range tests {
-// 		name := tt.name
-// 		t.Run(name, func(t *testing.T) {
-// 			creds, err := detect.DefaultCredentials(&detect.Options{
-// 				Scopes:   []string{"https://www.googleapis.com/auth/cloud-platform"},
-// 				CredentialsFile: tt.baseKeyFile,
-// 			})
-// 			if err != nil {
-// 				t.Fatalf("detect.DefaultCredentials() = %v", err)
-// 			}
-// 			aud := "http://example.com/"
-// 			tp, err := impersonate.NewIDTokenProvider(&impersonate.IDTokenOptions{
-// 				TargetPrincipal: writerEmail,
-// 				Audience:        aud,
-// 				Delegates:       tt.delegates,
-// 				IncludeEmail:    true,
-// 				TokenProvider:   creds,
-// 			})
-// 			if err != nil {
-// 				t.Fatalf("failed to create ts: %v", err)
-// 			}
-// 			tok, err := tp.Token(ctx)
-// 			if err != nil {
-// 				t.Fatalf("unable to retrieve Token: %v", err)
-// 			}
-// 			validTok, err := idtoken.Validate(ctx, tok.Value, aud)
-// 			if err != nil {
-// 				t.Fatalf("token validation failed: %v", err)
-// 			}
-// 			if validTok.Audience != aud {
-// 				t.Fatalf("got %q, want %q", validTok.Audience, aud)
-// 			}
-// 			if validTok.Claims["email"] != writerEmail {
-// 				t.Fatalf("got %q, want %q", validTok.Claims["email"], writerEmail)
-// 			}
-// 		})
-// 	}
-// }
+	for _, tt := range tests {
+		name := tt.name
+		t.Run(name, func(t *testing.T) {
+			creds, err := detect.DefaultCredentials(&detect.Options{
+				Scopes:          []string{"https://www.googleapis.com/auth/cloud-platform"},
+				CredentialsFile: tt.baseKeyFile,
+			})
+			if err != nil {
+				t.Fatalf("detect.DefaultCredentials() = %v", err)
+			}
+			aud := "http://example.com/"
+			tp, err := impersonate.NewIDTokenProvider(&impersonate.IDTokenOptions{
+				TargetPrincipal: writerEmail,
+				Audience:        aud,
+				Delegates:       tt.delegates,
+				IncludeEmail:    true,
+				TokenProvider:   creds,
+			})
+			if err != nil {
+				t.Fatalf("failed to create ts: %v", err)
+			}
+			tok, err := tp.Token(ctx)
+			if err != nil {
+				t.Fatalf("unable to retrieve Token: %v", err)
+			}
+			validTok, err := idtoken.Validate(ctx, tok.Value, aud)
+			if err != nil {
+				t.Fatalf("token validation failed: %v", err)
+			}
+			if validTok.Audience != aud {
+				t.Fatalf("got %q, want %q", validTok.Audience, aud)
+			}
+			if validTok.Claims["email"] != writerEmail {
+				t.Fatalf("got %q, want %q", validTok.Claims["email"], writerEmail)
+			}
+		})
+	}
+}
diff --git a/auth/internal/testutil/testdns/dns.go b/auth/internal/testutil/testdns/dns.go
@@ -23,8 +23,8 @@ import (
 	"net/http"
 
 	"cloud.google.com/go/auth"
+	"cloud.google.com/go/auth/httptransport"
 	"cloud.google.com/go/auth/internal"
-	"cloud.google.com/go/auth/internal/testutil"
 )
 
 // Client is a lightweight DNS client for testing.
@@ -36,7 +36,7 @@ type Client struct {
 // [cloud.google.com/go/auth.TokenProvider] for authentication.
 func NewClient(tp auth.TokenProvider) *Client {
 	client := internal.CloneDefaultClient()
-	testutil.AddAuthorizationMiddleware(client, tp)
+	httptransport.AddAuthorizationMiddleware(client, tp)
 	return &Client{
 		client: client,
 	}

diff --git a/auth/internal/testutil/testgcs/storage.go b/auth/internal/testutil/testgcs/storage.go
@@ -25,8 +25,8 @@ import (
 	"net/http"
 
 	"cloud.google.com/go/auth"
+	"cloud.google.com/go/auth/httptransport"
 	"cloud.google.com/go/auth/internal"
-	"cloud.google.com/go/auth/internal/testutil"
 )
 
 // Client is a lightweight GCS client for testing.
@@ -38,7 +38,7 @@ type Client struct {
 // [cloud.google.com/go/auth.TokenProvider] for authentication.
 func NewClient(tp auth.TokenProvider) *Client {
 	client := internal.CloneDefaultClient()
-	testutil.AddAuthorizationMiddleware(client, tp)
+	httptransport.AddAuthorizationMiddleware(client, tp)
 	return &Client{
 		client: client,
 	}

diff --git a/auth/internal/testutil/testutil.go b/auth/internal/testutil/testutil.go
@@ -15,12 +15,7 @@
 package testutil
 
 import (
-	"fmt"
-	"net/http"
 	"testing"
-
-	"cloud.google.com/go/auth"
-	"cloud.google.com/go/auth/internal"
 )
 
 // IntegrationTestCheck is a helper to check if an integration test should be
@@ -32,62 +27,3 @@ func IntegrationTestCheck(t *testing.T) {
 		t.Skip("skipping integration test")
 	}
 }
-
-// TODO(codyoss): remove all code below when httptransport package is added.
-
-// AddAuthorizationMiddleware adds a middleware to the provided client's
-// transport that sets the Authorization header with the value produced by the
-// provided [cloud.google.com/go/auth.TokenProvider]. An error is returned only
-// if client or tp is nil.
-func AddAuthorizationMiddleware(client *http.Client, tp auth.TokenProvider) error {
-	if client == nil || tp == nil {
-		return fmt.Errorf("httptransport: client and tp must not be nil")
-	}
-	base := client.Transport
-	if base == nil {
-		base = http.DefaultTransport.(*http.Transport).Clone()
-	}
-	client.Transport = &authTransport{
-		provider: auth.NewCachedTokenProvider(tp, nil),
-		base:     base,
-	}
-	return nil
-}
-
-type authTransport struct {
-	provider auth.TokenProvider
-	base     http.RoundTripper
-}
-
-// RoundTrip authorizes and authenticates the request with an
-// access token from Transport's Source. Per the RoundTripper contract we must
-// not modify the initial request, so we clone it, and we must close the body
-// on any errors that happens during our token logic.
-func (t *authTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	reqBodyClosed := false
-	if req.Body != nil {
-		defer func() {
-			if !reqBodyClosed {
-				req.Body.Close()
-			}
-		}()
-	}
-	token, err := t.provider.Token(req.Context())
-	if err != nil {
-		return nil, err
-	}
-	req2 := req.Clone(req.Context())
-	SetAuthHeader(token, req2)
-	reqBodyClosed = true
-	return t.base.RoundTrip(req2)
-}
-
-// SetAuthHeader uses the provided token to set the Authorization header on a
-// request. If the token.Type is empty, the type is assumed to be Bearer.
-func SetAuthHeader(token *auth.Token, req *http.Request) {
-	typ := token.Type
-	if typ == "" {
-		typ = internal.TokenTypeBearer
-	}
-	req.Header.Set("Authorization", typ+" "+token.Value)
-}