-
Notifications
You must be signed in to change notification settings - Fork 384
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
feat: Impersonated Universe Domain Support #1875
Conversation
…auth-library-nodejs into impersonated-universe
…js into impersonated-universe
// Create source client for impersonation | ||
const sourceClient = new UserRefreshClient(); | ||
sourceClient.fromJSON(json.source_credentials); | ||
const sourceClient = this.fromJSON(json.source_credentials); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is intentional to allow for impersonating any arbitrary type of source clients instead of just UserRefreshClient() right? Just for my own understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Other clients are able to use impersonation and its required in TPC (e.g. for external accounts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, only note I have is that the impersonation is handled directly by the external account client itself in this library (this already works for TPC, the inmpersonation URL gets generated with the universe domain when the create-credential-config is run in gCloud): https://github.com/googleapis/google-auth-library-nodejs/blob/a65d8a11450fdc0f69ea228def462e5a77beecd5/src/auth/baseexternalclient.ts#L602C4-L605C9
This PR adds support for a different type of credential configuration file which we should support, but just wanted to point out I don't think this is a path we would expect people to normally use for BYOID creds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Important call-out.
🦕