From 852a230c929a4a2614ecb1c485bcedd1b3f8bcae Mon Sep 17 00:00:00 2001 From: Cody Oss <6331106+codyoss@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:00:49 -0500 Subject: [PATCH] chore(auth): cleanup some small todos (#8628) fixup various small todos, still more to come --- auth/detect/compute.go | 2 - auth/detect/example_test.go | 52 ++++---- .../internal/externalaccount/aws_provider.go | 1 - auth/example_test.go | 20 +-- auth/impersonate/integration_test.go | 115 +++++++++--------- auth/internal/testutil/testdns/dns.go | 4 +- auth/internal/testutil/testgcs/storage.go | 4 +- auth/internal/testutil/testutil.go | 64 ---------- 8 files changed, 94 insertions(+), 168 deletions(-) diff --git a/auth/detect/compute.go b/auth/detect/compute.go index 510f5d291719..430026a98a0d 100644 --- 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 index a61f92a5bed8..1bcf64909cd6 100644 --- 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 index 888c80d30938..9b16a8d46c93 100644 --- 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 index d4beed7bd5f9..a1ad0c4dd988 100644 --- 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 index af995946a0db..337eb0ff9e8c 100644 --- 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 index fbc11ace4828..1939af9ea3c9 100644 --- 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 index 7bad231878a3..fb336fa9b38c 100644 --- 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 index 4dfbfce8083e..6f9ff7f019c4 100644 --- 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) -}