feat: add methods for fetching and using id tokens (second implementation) #867
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #867 +/- ##
=========================================
+ Coverage 90.76% 90.86% +0.1%
=========================================
Files 18 19 +1
Lines 4005 4150 +145
Branches 427 443 +16
=========================================
+ Hits 3635 3771 +136
- Misses 364 372 +8
- Partials 6 7 +1
Continue to review full report at Codecov.
|
…brary-nodejs into feat-id-tokens-2
samples/test/jwt.test.js
Outdated
| const url = | ||
| process.env.IAP_URL || 'https://nodejs-docs-samples-iap.appspot.com'; | ||
| const targetAudience = | ||
| process.env.IAP_CLIENT_ID || '170454875485-fbn7jalc9214bb67lslv1pbvmnijrb20.apps.googleusercontent.com'; |
There was a problem hiding this comment.
I wonder if we could get away with dropping this test, alternatively we either need to create a similar IAP resource in our CI/CD project, or add our CI/CD user to whatever project has that IAP resource.
There was a problem hiding this comment.
Seems like using CI/CD project would be better, but I don't have access to that so LMK what you'd like to do. I can remove it no problem.
|
Header lint checker is failing because of a bug. See googleapis/repo-automation-bots#223 |
| // limitations under the License. | ||
|
|
||
| 'use strict'; | ||
|
|
There was a problem hiding this comment.
For new samples, I guess we're adding a metadata block to the top of each sample file, like this:
// sample-metadata:
// title: Create Topic
// description: Creates a new topic.
// usage: node createTopic.js <topic-name>That might be a good addition here.
samples/idtokens-cloudrun.js
Outdated
| } | ||
|
|
||
| const args = process.argv.slice(2); | ||
| main(...args).catch(console.error); |
There was a problem hiding this comment.
I think the problem described in this issue may apply here:
| @@ -473,7 +476,10 @@ | |||
| "path": "samples/headers.js" | |||
| }, | |||
| { | |||
| "path": "samples/iap.js" | |||
| "path": "samples/idtokens-cloudrun.js" | |||
There was a problem hiding this comment.
this is auto generated, sorry that you ended up editing this by hand 😆
There was a problem hiding this comment.
This is looking good to me once you address @feywind's feedback; it looks like there's an upstream service causing the docs tests to pass, if you don't mind update linkinator.json to:
{
"recurse": true,
"skip": [
"https://codecov.io/gh/googleapis/",
"www.googleapis.com",
"https://david-dm.org.*"
]
}Which should get things passing for you.
| const jwt = new JWT({ | ||
| email: 'foo@serviceaccount.com', | ||
| key: PEM_CONTENTS, | ||
| subject: 'ignored@subjectaccount.com', |
There was a problem hiding this comment.
thanks for taking the time to add this test 👍
Codecov Report
@@ Coverage Diff @@
## master #867 +/- ##
==========================================
+ Coverage 90.76% 90.89% +0.13%
==========================================
Files 18 19 +1
Lines 4005 4151 +146
Branches 427 445 +18
==========================================
+ Hits 3635 3773 +138
- Misses 364 372 +8
Partials 6 6
Continue to review full report at Codecov.
|
This implementation is similar to #859, but uses an
IdTokenClientwrapper class. Additionally, the interfaceIdTokenProviderwas added to allow relevant clients to implement thefetchIdTokenmethod.Some thoughts:
fetchIdTokenmethod can be bypassed all together if the user just populatesadditionalClaimsin theJWTclass with the target audience. Do we care?OAuth2ClientinIdTokenClientand usingrefreshTokenNoCacheto callfetchIdTokenComputeclient directly to use ID Tokens (as you can withJWTusingadditionalClaims). This can be changed by adding atargetAudienceoption to the constructor, if it's desired.IdTokenClientyet! I wanted to clear the implementation first before writing them.Thanks!