google: panic in (TokenSource).Token() as returned by google.CredentialsFromJSONWithParams

[espadolini]

If a "Google Developers Console client_credentials.json" file is parsed by google.CredentialsFromJSONWithParams then the TokenSource for the Credentials returned is a TokenSource from the authhandler package, whose Token() method will ultimately call the params.AuthHandler callback without ever checking that it's non-nil. The only way to specify such a callback is through the params argument to CredentialsFromJSONWithParams or FindDefaultCredentialsWithParams, so there's no workaround if one were to use CredentialsFromJSON (which only gets some scopes as a string slice) or the DefaultTokenSource/FindDefaultCredentials functions.

Reproducible example:

package main

import (
	"context"

	"golang.org/x/oauth2/google"
)

func main() {
	cred, err := google.CredentialsFromJSON(context.TODO(), []byte(`{"web":{"redirect_uris":[""]}}`))
	if err != nil {
		return
	}
	cred.TokenSource.Token()
}

go version go1.19 darwin/arm64
golang.org/x/oauth2 v0.0.0-20220822191816-0ebed06d0094 (latest at time of writing)

The bug is present as far back as v0.0.0-20220411215720-9780585627b5, I haven't tried other platforms or versions other than those two, but the bug seems fairly straightforward to trigger.

FYI, it's also not very clear what the AuthHandler function is supposed to be doing, and there's no examples for it.

[jeromedoucet]

I will try to handle it and submit a PR.

[jeromedoucet]

@espadolini It seems that Token() implementation of authhandler TokenSource goal is to get a token through the 3 ledge Oauth flow.

The authHandler is supposed to handle the user consent and then return the authorization code. A example has been provided, but later deleted :

// CmdAuthorizationHandler returns a command line auth handler that prints
// the auth URL to the console and prompts the user to authorize in the
// browser and paste the auth code back via stdin.
//
// Per the OAuth protocol, a unique "state" string should be specified here.
// The authhandler token source will verify that the "state" is identical in
// the request and response before exchanging the auth code for OAuth token to
// prevent CSRF attacks.
//
// For convenience, this handler returns a pre-configured state instead of
// asking the user to additionally paste the state from the auth response.
// In order for this to work, the state configured here must match the state
// used in authCodeURL.
func CmdAuthorizationHandler(state string) authhandler.AuthorizationHandler {
       return func(authCodeURL string) (string, string, error) {
               fmt.Printf("Go to the following link in your browser:\n\n   %s\n\n", authCodeURL)
               fmt.Println("Enter authorization code:")
               var code string
               fmt.Scanln(&code)
               return code, state, nil
       }
}

func Example() {
       ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
               r.ParseForm()
               w.Header().Set("Content-Type", "application/json")
               w.Write([]byte(`{
                               "access_token": "90d64460d14870c08c81352a05dedd3465940a7c",
                               "scope": "pubsub",
                               "token_type": "bearer",
                               "expires_in": 3600
                       }`))
       }))
       defer ts.Close()

       ctx := context.Background()
       conf := &oauth2.Config{
               ClientID: "testClientID",
               Scopes:   []string{"pubsub"},
               Endpoint: oauth2.Endpoint{
                       AuthURL:  "testAuthCodeURL",
                       TokenURL: ts.URL,
               },
       }
       state := "unique_state"

       token, err := authhandler.TokenSource(ctx, conf, state, CmdAuthorizationHandler(state)).Token()

       if err != nil {
               fmt.Println(err)
       }

       fmt.Printf("AccessToken: %s", token.AccessToken)

       // Output:
       // Go to the following link in your browser:
       //
       //    testAuthCodeURL?client_id=testClientID&response_type=code&scope=pubsub&state=unique_state
       //
       // Enter authorization code:
       // AccessToken: 90d64460d14870c08c81352a05dedd3465940a7c
}

From my point of view, the best way to fix it, is to provide a default implementation of params.AuthHandler (based on that example, but with some improvements maybe) in google.CredentialsFromJSON().

I will begin to work on a PR tomorrow (it's late at home !), and we will see if the Go team agree with this fix.

[espadolini]

Maybe CredentialsFromJSONWithParams should error out if the passed file requires 3LO but params.AuthHandler is nil? An implementation that interacts with stdin and stdout (and blocks forever on stdin, even!) by default would be weird, IMO.

[jeromedoucet]

@espadolini I have to admit that this is not something ideal (Even if I think, reading the example, that it was design with that purposed in mind :/ ), but just returning an error is weird too.

Such situation is not a non nominal case, but a coding error, and may not be handle programmatically.

A panic is, IMO, a better choice here, but that will imply that CredentialsFromJSON will never work for 3LO which looks really weird.

I can imagine two ways of fixing that more or less properly :

1 panic if 3LO and nil handler + deprecation on CredentialsFromJSON (with meaningful message in the panic).
In that case, we admit that CredentialsFromJSON should be avoided, will never work as expected, and is about to disappear.

2 way to register a 3LO AuthHandler alongside CredentialsParams + panic if 3LO and all AuthHandler are nil
Here, we are trying to fix the design by finding a way to inject that AuthHandler after the Credentials solution.

The second one, is clearly the most complex, and I am unsure, considering the current design, that it is desirable : it seems that Credentials is not supposed to "know" something about the kind of flow its TokenFlow handle.

I am not very happy with the first one, but with the panic and the deprecation warning on CredentialsFromJSON, it seems to be the less bad :/