-
Notifications
You must be signed in to change notification settings - Fork 375
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: add methods for fetching and using id tokens (second implementation) #867
Conversation
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
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 is looking super solid ... as expected I think the refresh logic is going to add a few additional annoyances.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Cool, this looks overall pretty good to me. I am not as familiar with the subject matter, but it looks like Ben covered that. @bcoe back to you for final approvals?
// limitations under the License. | ||
|
||
'use strict'; | ||
|
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.
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.
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.
Added! PTAL
samples/idtokens-cloudrun.js
Outdated
} | ||
|
||
const args = process.argv.slice(2); | ||
main(...args).catch(console.error); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is auto generated, sorry that you ended up editing this by hand 😆
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 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: '[email protected]', | ||
key: PEM_CONTENTS, | ||
subject: '[email protected]', |
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 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
IdTokenClient
wrapper class. Additionally, the interfaceIdTokenProvider
was added to allow relevant clients to implement thefetchIdToken
method.Some thoughts:
fetchIdToken
method can be bypassed all together if the user just populatesadditionalClaims
in theJWT
class with the target audience. Do we care?OAuth2Client
inIdTokenClient
and usingrefreshTokenNoCache
to callfetchIdToken
Compute
client directly to use ID Tokens (as you can withJWT
usingadditionalClaims
). This can be changed by adding atargetAudience
option to the constructor, if it's desired.IdTokenClient
yet! I wanted to clear the implementation first before writing them.Thanks!