Specifying option.WithScopes is ignored when using option.WithTokenSource

[salrashid123]

If you use both

• https://pkg.go.dev/google.golang.org/api/option#WithScopes
• https://pkg.go.dev/google.golang.org/api/option#WithTokenSource

at the same time as shown below, the scopes settings from the tokensource is used.

The following works for cloudidentity api because the "books" api scope is simply ignored

    configci, err := google.JWTConfigFromJSON(serviceAccountJSON, cloudidentity.CloudPlatformScope, cloudidentity.CloudIdentityGroupsReadonlyScope)

    tsci := configci.TokenSource(ctx)

    cisvc, err := cloudidentity.NewService(ctx, option.WithTokenSource(tsci), option.WithScopes("https://www.googleapis.com/auth/books"))

but if you invert it, you'll see a scopes error since the actual token from the config is used

    configci, err := google.JWTConfigFromJSON(serviceAccountJSON, "https://www.googleapis.com/auth/books")
    tsci := configci.TokenSource(ctx)

    cisvc, err := cloudidentity.NewService(ctx, option.WithTokenSource(tsci), option.WithScopes(cloudidentity.CloudPlatformScope, cloudidentity.CloudIdentityGroupsReadonlyScope))

consider throwing an error if both are set

---

$ go version
go version go1.17.1 linux/amd64

module main

go 1.17

require (
	golang.org/x/oauth2 v0.0.0-20220722155238-128564f6959c
	google.golang.org/api v0.91.0
)

[codyoss]

The end behavior is intentional here, but I agree this could lead to hard to track down bugs. I think we should have a fast fail here and disallow passing both of these options at the same time as whatever is specified in the TS will always win.

[eisandbar]

It can be something analogous to this

google-api-go-client/internal/settings.go

Lines 107 to 109 in d6ee425

	if len(ds.Scopes) > 0 && len(ds.Audiences) > 0 {
	return errors.New("WithScopes is incompatible with WithAudience")
	}

if len(ds.Scopes) > 0 && ds.TokenSource != nil {
    return errors.New("WithScopes is incompatible with WithTokenSource")
}

[codyoss]

I commented this on the linked PR but wanted to make sure the same comment was captured in this issue as well

Although I think is a good change to make, it can't be made just yet. We will first need to audit our 
libraries to ensure we are not passing scopes via the WithScopes client option. I know where are in
clients in this repo and may still have places where this is done in our sister repo as well. Adding this
check would cause people to never be able to pass the TokenSource option right now in these
instances.

I also due worry thinking more about this that there may be libraries built on libraries where these
options are passed today. Once we have a better logging solution in place it may be better to log a
warning in cases like this. I will need to think about this a little more.

[salrashid123]

fwiw, i'd be happy if its just documented in the links above (no need at the moment to aggressively throw err)

[codyoss]

Yeah, I think that may be the best option for now. @eisandbar If you are willing to make that change I think that is a way we can move forward for now.

[eisandbar]

Should I add it to both WithScopes and WithTokenSource, or only WithScopes?

[codyoss]

I think just withscopes would be fine -- noting that it has no effect when used with tokensource opt.