From 5c1e6f48c0df0bd92189982e65c72f5d015d3e29 Mon Sep 17 00:00:00 2001 From: Radhika Agrawal Date: Tue, 27 Oct 2026 20:52:57 +0000 Subject: [PATCH 1/4] feat(util): add parallel cloud.google.com/go/auth options and handlers --- util/auth-handlers.go | 20 ++++- util/auth-handlers_test.go | 157 +++++++++++++++++++++++++++++++++++++ util/pkce.go | 20 ++++- util/pkce_test.go | 66 ++++++++++++++++ util/settings.go | 20 ++++- util/settings_test.go | 110 ++++++++++++++++++++++++++ 6 files changed, 383 insertions(+), 10 deletions(-) create mode 100644 util/auth-handlers_test.go create mode 100644 util/pkce_test.go create mode 100644 util/settings_test.go diff --git a/util/auth-handlers.go b/util/auth-handlers.go index ba50ca6e..33d3fcee 100644 --- a/util/auth-handlers.go +++ b/util/auth-handlers.go @@ -22,15 +22,16 @@ import ( "strings" "time" + "cloud.google.com/go/auth" "golang.org/x/oauth2/authhandler" ) -// 3LO authorization handler. Determines what algorithm to use -// to get the authorization code. +// Get3LOAuthHandler returns a 3LO authorization handler for cloud.google.com/go/auth. +// Determines what algorithm to use to get the authorization code. // // Note that the "state" parameter is used to prevent CSRF attacks. -func Get3LOAuthorizationHandler(state string, consentSettings ConsentPageSettings, - authCodeServer *AuthorizationCodeServer) authhandler.AuthorizationHandler { +func Get3LOAuthHandler(state string, consentSettings ConsentPageSettings, + authCodeServer *AuthorizationCodeServer) auth.AuthorizationHandler { return func(authCodeURL string) (string, string, error) { decodedValue, _ := url.ParseQuery(authCodeURL) redirectURL := decodedValue.Get("redirect_uri") @@ -43,6 +44,17 @@ func Get3LOAuthorizationHandler(state string, consentSettings ConsentPageSetting } } +// 3LO authorization handler. Determines what algorithm to use +// to get the authorization code. +// +// Note that the "state" parameter is used to prevent CSRF attacks. +// +// Deprecated: Use Get3LOAuthHandler instead. +func Get3LOAuthorizationHandler(state string, consentSettings ConsentPageSettings, + authCodeServer *AuthorizationCodeServer) authhandler.AuthorizationHandler { + return authhandler.AuthorizationHandler(Get3LOAuthHandler(state, consentSettings, authCodeServer)) +} + // authorization3LOOutOfBand prints the authorization URL on stdout // and reads the authorization code from stdin. // diff --git a/util/auth-handlers_test.go b/util/auth-handlers_test.go new file mode 100644 index 00000000..1ada99b2 --- /dev/null +++ b/util/auth-handlers_test.go @@ -0,0 +1,157 @@ +// +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package util + +import ( + "errors" + "os" + "testing" + "time" +) + +type fakeAuthCodeServer struct { + listening bool + authCode AuthorizationCode + authErr error + waitedConsent bool +} + +func (f *fakeAuthCodeServer) ListenAndServe(address string) (string, error) { + return address, nil +} + +func (f *fakeAuthCodeServer) Close() {} + +func (f *fakeAuthCodeServer) IsListeningAndServing() bool { + return f.listening +} + +func (f *fakeAuthCodeServer) WaitForListeningAndServing(maxWaitTime time.Duration) (bool, error) { + return f.listening, nil +} + +func (f *fakeAuthCodeServer) GetAuthenticationCode() (AuthorizationCode, error) { + return f.authCode, f.authErr +} + +func (f *fakeAuthCodeServer) WaitForConsentPageToReturnControl() error { + f.waitedConsent = true + return nil +} + +func TestGet3LOAuthHandlerLoopback(t *testing.T) { + fakeSrv := &fakeAuthCodeServer{ + listening: true, + authCode: AuthorizationCode{ + Code: "loopback-code", + State: "loopback-state", + }, + } + var srv AuthorizationCodeServer = fakeSrv + consentSettings := ConsentPageSettings{ + DisableAutoOpenConsentPage: true, + InteractionTimeout: time.Second, + } + + handler := Get3LOAuthHandler("unused-oob-state", consentSettings, &srv) + authURL := "https://accounts.google.com/o/oauth2/auth?client_id=test&redirect_uri=http%3A%2F%2Flocalhost%3A8080" + + code, state, err := handler(authURL) + if err != nil { + t.Fatalf("Get3LOAuthHandler() unexpected error = %v", err) + } + if !fakeSrv.waitedConsent { + t.Error("expected WaitForConsentPageToReturnControl to be called") + } + if code != "loopback-code" || state != "loopback-state" { + t.Errorf("Get3LOAuthHandler() = (%q, %q), want (%q, %q)", code, state, "loopback-code", "loopback-state") + } +} + +func TestGet3LOAuthHandlerLoopbackError(t *testing.T) { + wantErr := errors.New("consent denied") + fakeSrv := &fakeAuthCodeServer{ + listening: false, + authErr: wantErr, + } + var srv AuthorizationCodeServer = fakeSrv + consentSettings := ConsentPageSettings{ + DisableAutoOpenConsentPage: true, + } + + handler := Get3LOAuthHandler("state", consentSettings, &srv) + authURL := "https://accounts.google.com/o/oauth2/auth?client_id=test&redirect_uri=http%3A%2F%2Flocalhost%3A8080" + + _, _, err := handler(authURL) + if !errors.Is(err, wantErr) { + t.Fatalf("Get3LOAuthHandler() error = %v, want %v", err, wantErr) + } +} + +func TestGet3LOAuthHandlerOutOfBand(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe() failed: %v", err) + } + origStdin := os.Stdin + os.Stdin = r + defer func() { + os.Stdin = origStdin + r.Close() + }() + + if _, err := w.WriteString("oob-auth-code\n"); err != nil { + t.Fatalf("WriteString() failed: %v", err) + } + w.Close() + + var srv AuthorizationCodeServer + handler := Get3LOAuthHandler("oob-state", ConsentPageSettings{}, &srv) + authURL := "https://accounts.google.com/o/oauth2/auth?client_id=test&redirect_uri=urn%3Aietf%3Awg%3Aoauth%3A2.0%3Aoob" + + code, state, err := handler(authURL) + if err != nil { + t.Fatalf("Get3LOAuthHandler() unexpected error = %v", err) + } + if code != "oob-auth-code" || state != "oob-state" { + t.Errorf("Get3LOAuthHandler() = (%q, %q), want (%q, %q)", code, state, "oob-auth-code", "oob-state") + } +} + +func TestGet3LOAuthorizationHandlerDelegatesToGet3LOAuthHandler(t *testing.T) { + fakeSrv := &fakeAuthCodeServer{ + listening: true, + authCode: AuthorizationCode{ + Code: "legacy-loopback-code", + State: "legacy-loopback-state", + }, + } + var srv AuthorizationCodeServer = fakeSrv + consentSettings := ConsentPageSettings{ + DisableAutoOpenConsentPage: true, + InteractionTimeout: time.Second, + } + + handler := Get3LOAuthorizationHandler("state", consentSettings, &srv) + authURL := "https://accounts.google.com/o/oauth2/auth?client_id=test&redirect_uri=http%3A%2F%2Flocalhost%3A8080" + + code, state, err := handler(authURL) + if err != nil { + t.Fatalf("Get3LOAuthorizationHandler() unexpected error = %v", err) + } + if code != "legacy-loopback-code" || state != "legacy-loopback-state" { + t.Errorf("Get3LOAuthorizationHandler() = (%q, %q), want (%q, %q)", code, state, "legacy-loopback-code", "legacy-loopback-state") + } +} diff --git a/util/pkce.go b/util/pkce.go index b60b29ba..7bf309db 100644 --- a/util/pkce.go +++ b/util/pkce.go @@ -18,20 +18,34 @@ import ( "crypto/sha256" "encoding/base64" + "cloud.google.com/go/auth" "github.com/google/uuid" "golang.org/x/oauth2/authhandler" ) -// GeneratePKCEParams generates a unique PKCE challenge and verifier combination, +// GeneratePKCEOptions generates a unique PKCE challenge and verifier combination, // using UUID, SHA256 encryption, and base64 URL encoding with no padding. -func GeneratePKCEParams() *authhandler.PKCEParams { +func GeneratePKCEOptions() *auth.PKCEOptions { verifier := uuid.New().String() sha := sha256.Sum256([]byte(verifier)) challenge := base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(sha[:]) - return &authhandler.PKCEParams{ + return &auth.PKCEOptions{ Challenge: challenge, ChallengeMethod: "S256", Verifier: verifier, } } + +// GeneratePKCEParams generates a unique PKCE challenge and verifier combination, +// using UUID, SHA256 encryption, and base64 URL encoding with no padding. +// +// Deprecated: Use GeneratePKCEOptions instead. +func GeneratePKCEParams() *authhandler.PKCEParams { + opts := GeneratePKCEOptions() + return &authhandler.PKCEParams{ + Challenge: opts.Challenge, + ChallengeMethod: opts.ChallengeMethod, + Verifier: opts.Verifier, + } +} diff --git a/util/pkce_test.go b/util/pkce_test.go new file mode 100644 index 00000000..f63d4ca7 --- /dev/null +++ b/util/pkce_test.go @@ -0,0 +1,66 @@ +// +// Copyright 2022 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package util + +import ( + "crypto/sha256" + "encoding/base64" + "testing" + + "github.com/google/uuid" +) + +func TestGeneratePKCEOptions(t *testing.T) { + opts1 := GeneratePKCEOptions() + if opts1 == nil { + t.Fatal("GeneratePKCEOptions() = nil, want non-nil") + } + if opts1.ChallengeMethod != "S256" { + t.Errorf("ChallengeMethod = %q, want %q", opts1.ChallengeMethod, "S256") + } + if _, err := uuid.Parse(opts1.Verifier); err != nil { + t.Errorf("Verifier %q is not a valid UUID: %v", opts1.Verifier, err) + } + + sha := sha256.Sum256([]byte(opts1.Verifier)) + wantChallenge := base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(sha[:]) + if opts1.Challenge != wantChallenge { + t.Errorf("Challenge = %q, want %q", opts1.Challenge, wantChallenge) + } + + opts2 := GeneratePKCEOptions() + if opts1.Verifier == opts2.Verifier { + t.Errorf("consecutive GeneratePKCEOptions() calls returned identical Verifier %q", opts1.Verifier) + } +} + +func TestGeneratePKCEParams(t *testing.T) { + params := GeneratePKCEParams() + if params == nil { + t.Fatal("GeneratePKCEParams() = nil, want non-nil") + } + if params.ChallengeMethod != "S256" { + t.Errorf("ChallengeMethod = %q, want %q", params.ChallengeMethod, "S256") + } + if _, err := uuid.Parse(params.Verifier); err != nil { + t.Errorf("Verifier %q is not a valid UUID: %v", params.Verifier, err) + } + + sha := sha256.Sum256([]byte(params.Verifier)) + wantChallenge := base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(sha[:]) + if params.Challenge != wantChallenge { + t.Errorf("Challenge = %q, want %q", params.Challenge, wantChallenge) + } +} diff --git a/util/settings.go b/util/settings.go index b4f76a80..2d5d5dda 100644 --- a/util/settings.go +++ b/util/settings.go @@ -15,9 +15,8 @@ package util import ( -// Adding blank lines for go mod tidy and vendor -// will fix in Phase2 PR for migration to go/auth. - _ "cloud.google.com/go/auth" + "cloud.google.com/go/auth" + // Blank imports for go mod tidy and vendor; will be imported directly in subsequent migration PRs. _ "cloud.google.com/go/auth/credentials" _ "cloud.google.com/go/auth/oauth2adapt" "golang.org/x/oauth2/authhandler" @@ -50,7 +49,12 @@ type Settings struct { // billing charges. QuotaProject string // AuthHandler is the AuthorizationHandler used for 3-legged OAuth flow. + // + // Deprecated: Use GoogleAuthHandler instead. AuthHandler authhandler.AuthorizationHandler + // GoogleAuthHandler is the cloud.google.com/go/auth AuthorizationHandler + // used for 3-legged OAuth flow. + GoogleAuthHandler auth.AuthorizationHandler // State is a unique string used with AuthHandler. State string // Indicates that STS token exchange should be performed. @@ -60,6 +64,16 @@ type Settings struct { ServiceAccount string } +func (s Settings) resolveAuthHandler() auth.AuthorizationHandler { + if s.GoogleAuthHandler != nil { + return s.GoogleAuthHandler + } + if s.AuthHandler != nil { + return auth.AuthorizationHandler(s.AuthHandler) + } + return nil +} + func (s Settings) GetAuthType() string { if s.AuthType != "" { return s.AuthType diff --git a/util/settings_test.go b/util/settings_test.go new file mode 100644 index 00000000..ba72f748 --- /dev/null +++ b/util/settings_test.go @@ -0,0 +1,110 @@ +// +// Copyright 2021 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package util + +import ( + "testing" + + "cloud.google.com/go/auth" + "golang.org/x/oauth2/authhandler" +) + +func TestResolveAuthHandler(t *testing.T) { + googleHandler := auth.AuthorizationHandler(func(url string) (string, string, error) { + return "google-code", "google-state", nil + }) + legacyHandler := authhandler.AuthorizationHandler(func(url string) (string, string, error) { + return "legacy-code", "legacy-state", nil + }) + + t.Run("prefers GoogleAuthHandler when set", func(t *testing.T) { + s := Settings{ + GoogleAuthHandler: googleHandler, + AuthHandler: legacyHandler, + } + h := s.resolveAuthHandler() + if h == nil { + t.Fatal("resolveAuthHandler() = nil, want non-nil") + } + code, state, err := h("https://accounts.google.com/o/oauth2/auth") + if err != nil { + t.Fatalf("handler returned unexpected error: %v", err) + } + if code != "google-code" || state != "google-state" { + t.Errorf("handler() = (%q, %q), want (%q, %q)", code, state, "google-code", "google-state") + } + }) + + t.Run("falls back to deprecated AuthHandler when GoogleAuthHandler is nil", func(t *testing.T) { + s := Settings{ + AuthHandler: legacyHandler, + } + h := s.resolveAuthHandler() + if h == nil { + t.Fatal("resolveAuthHandler() = nil, want non-nil") + } + code, state, err := h("https://accounts.google.com/o/oauth2/auth") + if err != nil { + t.Fatalf("handler returned unexpected error: %v", err) + } + if code != "legacy-code" || state != "legacy-state" { + t.Errorf("handler() = (%q, %q), want (%q, %q)", code, state, "legacy-code", "legacy-state") + } + }) + + t.Run("returns nil when neither handler is set", func(t *testing.T) { + var s Settings + if h := s.resolveAuthHandler(); h != nil { + t.Errorf("resolveAuthHandler() = %v, want nil", h) + } + }) +} + +func TestGetAuthType(t *testing.T) { + tests := []struct { + name string + settings Settings + want string + }{ + { + name: "explicit AuthType takes precedence", + settings: Settings{AuthType: AuthTypeSSO, APIKey: "key", Scope: "scope"}, + want: AuthTypeSSO, + }, + { + name: "APIKey selects apikey auth type", + settings: Settings{APIKey: "my-api-key"}, + want: AuthTypeAPIKey, + }, + { + name: "Scope selects oauth auth type", + settings: Settings{Scope: "https://www.googleapis.com/auth/cloud-platform"}, + want: AuthTypeOAuth, + }, + { + name: "defaults to jwt auth type", + settings: Settings{Audience: "https://example.googleapis.com/"}, + want: AuthTypeJWT, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.settings.GetAuthType(); got != tc.want { + t.Errorf("GetAuthType() = %q, want %q", got, tc.want) + } + }) + } +} From 1712af0fffc88cc4254cbe76fb099bd4122670c4 Mon Sep 17 00:00:00 2001 From: Radhika Agrawal Date: Tue, 27 Oct 2026 21:42:49 +0000 Subject: [PATCH 2/4] fix(util): address PR review feedback and wire resolveAuthHandler into fetch --- util/auth-handlers.go | 5 +- util/auth-handlers_test.go | 9 ++- util/fetch.go | 5 +- util/pkce_test.go | 13 ++-- util/settings.go | 11 ++-- util/settings_test.go | 119 ++++++++++++++++++++++++------------- 6 files changed, 102 insertions(+), 60 deletions(-) diff --git a/util/auth-handlers.go b/util/auth-handlers.go index 33d3fcee..d7309d21 100644 --- a/util/auth-handlers.go +++ b/util/auth-handlers.go @@ -44,8 +44,9 @@ func Get3LOAuthHandler(state string, consentSettings ConsentPageSettings, } } -// 3LO authorization handler. Determines what algorithm to use -// to get the authorization code. +// Get3LOAuthorizationHandler returns a 3LO authorization handler for +// golang.org/x/oauth2/authhandler. Determines what algorithm to use to get +// the authorization code. // // Note that the "state" parameter is used to prevent CSRF attacks. // diff --git a/util/auth-handlers_test.go b/util/auth-handlers_test.go index 1ada99b2..5ce494ec 100644 --- a/util/auth-handlers_test.go +++ b/util/auth-handlers_test.go @@ -1,5 +1,5 @@ // -// Copyright 2020 Google Inc. +// Copyright 2026 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -83,23 +83,28 @@ func TestGet3LOAuthHandlerLoopback(t *testing.T) { func TestGet3LOAuthHandlerLoopbackError(t *testing.T) { wantErr := errors.New("consent denied") fakeSrv := &fakeAuthCodeServer{ - listening: false, + listening: true, authErr: wantErr, } var srv AuthorizationCodeServer = fakeSrv consentSettings := ConsentPageSettings{ DisableAutoOpenConsentPage: true, + InteractionTimeout: time.Second, } handler := Get3LOAuthHandler("state", consentSettings, &srv) authURL := "https://accounts.google.com/o/oauth2/auth?client_id=test&redirect_uri=http%3A%2F%2Flocalhost%3A8080" _, _, err := handler(authURL) + if !fakeSrv.waitedConsent { + t.Error("expected WaitForConsentPageToReturnControl to be called") + } if !errors.Is(err, wantErr) { t.Fatalf("Get3LOAuthHandler() error = %v, want %v", err, wantErr) } } +// TestGet3LOAuthHandlerOutOfBand mutates process-global os.Stdin; do not mark t.Parallel(). func TestGet3LOAuthHandlerOutOfBand(t *testing.T) { r, w, err := os.Pipe() if err != nil { diff --git a/util/fetch.go b/util/fetch.go index 9e47e08f..80eeb985 100644 --- a/util/fetch.go +++ b/util/fetch.go @@ -20,7 +20,10 @@ import ( "fmt" "strings" + // Blank import for go mod tidy and vendor; will be imported directly in Phase 4. + _ "cloud.google.com/go/auth/oauth2adapt" "golang.org/x/oauth2" + "golang.org/x/oauth2/authhandler" "golang.org/x/oauth2/google" ) @@ -94,7 +97,7 @@ func FindJSONCredentials(ctx context.Context, settings *Settings) (*google.Crede var params google.CredentialsParams params.Scopes = strings.Split(settings.Scope, " ") params.State = "state" - params.AuthHandler = settings.AuthHandler + params.AuthHandler = authhandler.AuthorizationHandler(settings.resolveAuthHandler()) params.Subject = settings.Email params.PKCE = GeneratePKCEParams() if settings.CredentialsJSON != "" { diff --git a/util/pkce_test.go b/util/pkce_test.go index f63d4ca7..00465c82 100644 --- a/util/pkce_test.go +++ b/util/pkce_test.go @@ -1,5 +1,5 @@ // -// Copyright 2022 Google Inc. +// Copyright 2026 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -54,13 +54,10 @@ func TestGeneratePKCEParams(t *testing.T) { if params.ChallengeMethod != "S256" { t.Errorf("ChallengeMethod = %q, want %q", params.ChallengeMethod, "S256") } - if _, err := uuid.Parse(params.Verifier); err != nil { - t.Errorf("Verifier %q is not a valid UUID: %v", params.Verifier, err) + if params.Verifier == "" { + t.Error("Verifier is empty, want non-empty") } - - sha := sha256.Sum256([]byte(params.Verifier)) - wantChallenge := base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(sha[:]) - if params.Challenge != wantChallenge { - t.Errorf("Challenge = %q, want %q", params.Challenge, wantChallenge) + if params.Challenge == "" { + t.Error("Challenge is empty, want non-empty") } } diff --git a/util/settings.go b/util/settings.go index 2d5d5dda..4d1c6ad5 100644 --- a/util/settings.go +++ b/util/settings.go @@ -15,11 +15,11 @@ package util import ( - "cloud.google.com/go/auth" - // Blank imports for go mod tidy and vendor; will be imported directly in subsequent migration PRs. + // Blank import for go mod tidy and vendor; will be imported directly in util/fetch.go. _ "cloud.google.com/go/auth/credentials" - _ "cloud.google.com/go/auth/oauth2adapt" "golang.org/x/oauth2/authhandler" + + "cloud.google.com/go/auth" ) var AuthTypeOAuth = "oauth" @@ -53,9 +53,10 @@ type Settings struct { // Deprecated: Use GoogleAuthHandler instead. AuthHandler authhandler.AuthorizationHandler // GoogleAuthHandler is the cloud.google.com/go/auth AuthorizationHandler - // used for 3-legged OAuth flow. + // used for 3-legged OAuth flow. It takes precedence over AuthHandler when + // both are set. GoogleAuthHandler auth.AuthorizationHandler - // State is a unique string used with AuthHandler. + // State is a unique string used with GoogleAuthHandler (or the deprecated AuthHandler). State string // Indicates that STS token exchange should be performed. Sts bool diff --git a/util/settings_test.go b/util/settings_test.go index ba72f748..8e98501c 100644 --- a/util/settings_test.go +++ b/util/settings_test.go @@ -1,5 +1,5 @@ // -// Copyright 2021 Google Inc. +// Copyright 2026 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,6 +15,8 @@ package util import ( + "context" + "errors" "testing" "cloud.google.com/go/auth" @@ -22,54 +24,87 @@ import ( ) func TestResolveAuthHandler(t *testing.T) { - googleHandler := auth.AuthorizationHandler(func(url string) (string, string, error) { + googleHandler := auth.AuthorizationHandler(func(string) (string, string, error) { return "google-code", "google-state", nil }) - legacyHandler := authhandler.AuthorizationHandler(func(url string) (string, string, error) { + legacyHandler := authhandler.AuthorizationHandler(func(string) (string, string, error) { return "legacy-code", "legacy-state", nil }) - t.Run("prefers GoogleAuthHandler when set", func(t *testing.T) { - s := Settings{ - GoogleAuthHandler: googleHandler, - AuthHandler: legacyHandler, - } - h := s.resolveAuthHandler() - if h == nil { - t.Fatal("resolveAuthHandler() = nil, want non-nil") - } - code, state, err := h("https://accounts.google.com/o/oauth2/auth") - if err != nil { - t.Fatalf("handler returned unexpected error: %v", err) - } - if code != "google-code" || state != "google-state" { - t.Errorf("handler() = (%q, %q), want (%q, %q)", code, state, "google-code", "google-state") - } - }) + tests := []struct { + name string + settings Settings + wantCode, wantState string + }{ + { + name: "prefers GoogleAuthHandler when both set", + settings: Settings{GoogleAuthHandler: googleHandler, AuthHandler: legacyHandler}, + wantCode: "google-code", + wantState: "google-state", + }, + { + name: "falls back to deprecated AuthHandler", + settings: Settings{AuthHandler: legacyHandler}, + wantCode: "legacy-code", + wantState: "legacy-state", + }, + } - t.Run("falls back to deprecated AuthHandler when GoogleAuthHandler is nil", func(t *testing.T) { - s := Settings{ - AuthHandler: legacyHandler, - } - h := s.resolveAuthHandler() - if h == nil { - t.Fatal("resolveAuthHandler() = nil, want non-nil") - } - code, state, err := h("https://accounts.google.com/o/oauth2/auth") - if err != nil { - t.Fatalf("handler returned unexpected error: %v", err) - } - if code != "legacy-code" || state != "legacy-state" { - t.Errorf("handler() = (%q, %q), want (%q, %q)", code, state, "legacy-code", "legacy-state") - } - }) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h := tc.settings.resolveAuthHandler() + if h == nil { + t.Fatal("resolveAuthHandler() = nil, want non-nil") + } + code, state, err := h("https://accounts.google.com/o/oauth2/auth") + if err != nil { + t.Fatalf("handler returned unexpected error: %v", err) + } + if code != tc.wantCode || state != tc.wantState { + t.Errorf("handler() = (%q, %q), want (%q, %q)", code, state, tc.wantCode, tc.wantState) + } + }) + } +} - t.Run("returns nil when neither handler is set", func(t *testing.T) { - var s Settings - if h := s.resolveAuthHandler(); h != nil { - t.Errorf("resolveAuthHandler() = %v, want nil", h) - } - }) +func TestResolveAuthHandlerUnset(t *testing.T) { + var s Settings + if h := s.resolveAuthHandler(); h != nil { + t.Errorf("resolveAuthHandler() = %v, want nil", h) + } +} + +func TestFindJSONCredentialsHonorsGoogleAuthHandler(t *testing.T) { + wantErr := errors.New("stop after handler invoked") + called := false + s := &Settings{ + CredentialsJSON: `{ + "installed": { + "client_id": "test-client.apps.googleusercontent.com", + "client_secret": "test-secret", + "auth_uri": "https://accounts.google.com/o/oauth2/auth", + "token_uri": "https://oauth2.googleapis.com/token", + "redirect_uris": ["http://localhost"] + } +}`, + Scope: "https://www.googleapis.com/auth/cloud-platform", + GoogleAuthHandler: func(authCodeURL string) (string, string, error) { + called = true + return "", "", wantErr + }, + } + + creds, err := FindJSONCredentials(context.Background(), s) + if err != nil { + t.Fatalf("FindJSONCredentials() unexpected error: %v", err) + } + _, err = creds.TokenSource.Token() + if !called { + t.Fatal("expected GoogleAuthHandler to be invoked by TokenSource.Token()") + } + if !errors.Is(err, wantErr) { + t.Errorf("TokenSource.Token() error = %v, want %v", err, wantErr) + } } func TestGetAuthType(t *testing.T) { From e902d2cca37ab0265a7914aa12dcb2b9964e99c0 Mon Sep 17 00:00:00 2001 From: Radhika Agrawal Date: Wed, 28 Oct 2026 19:28:26 +0000 Subject: [PATCH 3/4] fix(util): address follow-up review feedback on imports and test coverage --- util/auth-handlers_test.go | 46 +++++++++++++++++++++++++++++++++++++- util/fetch.go | 3 +++ util/settings.go | 3 +-- util/settings_test.go | 6 +++++ 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/util/auth-handlers_test.go b/util/auth-handlers_test.go index 5ce494ec..1fc76109 100644 --- a/util/auth-handlers_test.go +++ b/util/auth-handlers_test.go @@ -51,7 +51,24 @@ func (f *fakeAuthCodeServer) WaitForConsentPageToReturnControl() error { return nil } +// silenceStdout redirects process-global os.Stdout to os.DevNull for the duration +// of a test. Tests calling this helper must not be marked t.Parallel(). +func silenceStdout(t *testing.T) { + t.Helper() + devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + t.Fatalf("os.OpenFile(%q) failed: %v", os.DevNull, err) + } + origStdout := os.Stdout + os.Stdout = devNull + t.Cleanup(func() { + os.Stdout = origStdout + devNull.Close() + }) +} + func TestGet3LOAuthHandlerLoopback(t *testing.T) { + silenceStdout(t) fakeSrv := &fakeAuthCodeServer{ listening: true, authCode: AuthorizationCode{ @@ -81,6 +98,7 @@ func TestGet3LOAuthHandlerLoopback(t *testing.T) { } func TestGet3LOAuthHandlerLoopbackError(t *testing.T) { + silenceStdout(t) wantErr := errors.New("consent denied") fakeSrv := &fakeAuthCodeServer{ listening: true, @@ -104,8 +122,33 @@ func TestGet3LOAuthHandlerLoopbackError(t *testing.T) { } } -// TestGet3LOAuthHandlerOutOfBand mutates process-global os.Stdin; do not mark t.Parallel(). +func TestGet3LOAuthHandlerLoopbackServerNotListening(t *testing.T) { + wantErr := errors.New("server failed to start") + fakeSrv := &fakeAuthCodeServer{ + listening: false, + authErr: wantErr, + } + var srv AuthorizationCodeServer = fakeSrv + consentSettings := ConsentPageSettings{ + DisableAutoOpenConsentPage: true, + InteractionTimeout: time.Second, + } + + handler := Get3LOAuthHandler("state", consentSettings, &srv) + authURL := "https://accounts.google.com/o/oauth2/auth?client_id=test&redirect_uri=http%3A%2F%2Flocalhost%3A8080" + + _, _, err := handler(authURL) + if fakeSrv.waitedConsent { + t.Error("did not expect WaitForConsentPageToReturnControl to be called when server is not listening") + } + if !errors.Is(err, wantErr) { + t.Fatalf("Get3LOAuthHandler() error = %v, want %v", err, wantErr) + } +} + +// TestGet3LOAuthHandlerOutOfBand mutates process-global os.Stdin and os.Stdout; do not mark t.Parallel(). func TestGet3LOAuthHandlerOutOfBand(t *testing.T) { + silenceStdout(t) r, w, err := os.Pipe() if err != nil { t.Fatalf("os.Pipe() failed: %v", err) @@ -136,6 +179,7 @@ func TestGet3LOAuthHandlerOutOfBand(t *testing.T) { } func TestGet3LOAuthorizationHandlerDelegatesToGet3LOAuthHandler(t *testing.T) { + silenceStdout(t) fakeSrv := &fakeAuthCodeServer{ listening: true, authCode: AuthorizationCode{ diff --git a/util/fetch.go b/util/fetch.go index 80eeb985..d364ef33 100644 --- a/util/fetch.go +++ b/util/fetch.go @@ -97,6 +97,9 @@ func FindJSONCredentials(ctx context.Context, settings *Settings) (*google.Crede var params google.CredentialsParams params.Scopes = strings.Split(settings.Scope, " ") params.State = "state" + // Convert the resolved handler back to authhandler.AuthorizationHandler and + // use GeneratePKCEParams() while FindJSONCredentials still constructs + // google.CredentialsParams; Phase 4 replaces this with FindAuthCredentials. params.AuthHandler = authhandler.AuthorizationHandler(settings.resolveAuthHandler()) params.Subject = settings.Email params.PKCE = GeneratePKCEParams() diff --git a/util/settings.go b/util/settings.go index 4d1c6ad5..20e9a628 100644 --- a/util/settings.go +++ b/util/settings.go @@ -15,11 +15,10 @@ package util import ( + "cloud.google.com/go/auth" // Blank import for go mod tidy and vendor; will be imported directly in util/fetch.go. _ "cloud.google.com/go/auth/credentials" "golang.org/x/oauth2/authhandler" - - "cloud.google.com/go/auth" ) var AuthTypeOAuth = "oauth" diff --git a/util/settings_test.go b/util/settings_test.go index 8e98501c..7c01c308 100644 --- a/util/settings_test.go +++ b/util/settings_test.go @@ -42,6 +42,12 @@ func TestResolveAuthHandler(t *testing.T) { wantCode: "google-code", wantState: "google-state", }, + { + name: "uses GoogleAuthHandler when only GoogleAuthHandler set", + settings: Settings{GoogleAuthHandler: googleHandler}, + wantCode: "google-code", + wantState: "google-state", + }, { name: "falls back to deprecated AuthHandler", settings: Settings{AuthHandler: legacyHandler}, From 5ffae5f3103a716514ec1c0607907434c8119c2b Mon Sep 17 00:00:00 2001 From: Radhika Agrawal Date: Wed, 28 Oct 2026 20:03:50 +0000 Subject: [PATCH 4/4] docs(util): note temporary authhandler import in fetch.go until Phase 4 --- util/fetch.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/fetch.go b/util/fetch.go index d364ef33..4b13910e 100644 --- a/util/fetch.go +++ b/util/fetch.go @@ -23,6 +23,9 @@ import ( // Blank import for go mod tidy and vendor; will be imported directly in Phase 4. _ "cloud.google.com/go/auth/oauth2adapt" "golang.org/x/oauth2" + // Temporary import to convert settings.resolveAuthHandler() for google.CredentialsParams; + // will be removed in the next phase (Phase 4) when google.CredentialsParams is replaced + // with credentials.DetectOptions. "golang.org/x/oauth2/authhandler" "golang.org/x/oauth2/google" )