Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GoogleAuth ignores credentials passed to it in getClient in favor of cached credentials #390

Closed
G-Rath opened this issue Jun 21, 2018 · 4 comments · Fixed by #749
Closed
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@G-Rath
Copy link

G-Rath commented Jun 21, 2018

The getClient method in GoogleAuth will ignore any credentials passed to it if it has cached credentials, regardless of differences between the credentials passed in the parameter and the credentials that have been cached:

https://github.com/google/google-auth-library-nodejs/blob/0ff24c7c4a1c5da604dd2019dfc225a922934e3e/src/auth/googleauth.ts#L656-L675

As far as I can find, there is no way to actually clear the cached credentials once it has been set. (Aside from manually explicitly setting the property to null, which doesn't feel right to me).

I'd imagine ideally that if the credentials didn't match, the cached ones would be ignored.
I'd expect at least some way to bust the cache; possibly as a property in the options parameter (something like useCachedCredentials: boolean).

@JustinBeckwith JustinBeckwith added triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jun 21, 2018
@JustinBeckwith
Copy link
Contributor

Greetings! This does look like a bug. As a workaround, you can instantiate a new GoogleAuth object:

const auth = new GoogleAuth({
 ...
});

We will take a look at a proper fix!

@JustinBeckwith JustinBeckwith removed the triage me I really want to be triaged. label Jun 21, 2018
@G-Rath
Copy link
Author

G-Rath commented Jun 22, 2018

Thank you.

That was the solution I came up with as well:

this._promiseGoogleAuthClient = (new GoogleAuth({
    credentials: this._dialogflowCredentials,
    scopes: this._requiredGoogleScopes
})).getClient(/* options here are always ignored b/c cache */);

@eyqs
Copy link

eyqs commented Jun 28, 2018

How would you tell if the credentials match without reading in the content? Would you just compare their keyFilenames?

@filecage
Copy link

filecage commented Dec 26, 2018

Stumbled upon the same issue when using googleapis. I don't see any reason why this library should cache the auth client except from premature optimization. I suggest that the getClient method should never cache - even if you pass the exact same credentials the caching should be handled in userland.

If caching the auth is a necessary feature however, I propose the following (breaking) changes:

  • rename getClient to getClientCached to make it clear that this method will cache the client (would have saved me at least 2 hours today)
  • add a new method createClient that always returns a new auth client

I assume that removing the cache would be a breaking change anyway, because people's applications might slow down.

Edit: After digging a bit deeper I partly revoke my proposal. Memoizing the client inside the GoogleAuth class is totally fine in my eyes while the naming of the methods of the googleapis.auth object are not. As this is mainly an issue with a) the nature of singletons and b) googleapis decision to use this class as a singleton my previous suggestion might be the wrong thing to do (while I still think that the naming could be improved).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants