-
Notifications
You must be signed in to change notification settings - Fork 373
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
Adding in alpha interface for blocking token verification #1635
Conversation
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.
Thank you, @colerogers !
Looks good so far. I left a few comments. Please run npm run api-extractor:local
to fix the CI errors.
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 @colerogers ! Looks pretty good. Added a few more comments.
test/resources/mocks.ts
Outdated
export function generateAuthBlockingToken(overrides?: object, claims?: object): string { | ||
const options = _.assign({ | ||
audience: `https://us-central1-${projectId}.cloudfunctions.net/functionName`, | ||
expiresIn: ONE_HOUR_IN_SECONDS, |
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.
Just confirming. Is 1 hour the intended expiry time or do we want a custom/shorter duration 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.
Looked at the DD and changed it to ten minutes
.then((customToken) => { | ||
return tokenVerifierSessionCookie._verifyAuthBlockingToken(customToken, false, undefined) | ||
.should.eventually.be.rejectedWith( | ||
'verifySessionCookie() expects a session cookie, but was given a custom token'); |
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.
Does _verifyAuthBlockingToken()
expects a session cookie 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.
Ahh, I copied this test from verifyIdToken, removed it in the latest push
|
||
return tokenVerifierSessionCookie._verifyAuthBlockingToken(legacyCustomToken, false, undefined) | ||
.should.eventually.be.rejectedWith( | ||
'verifySessionCookie() expects a session cookie, but was given a legacy custom token'); |
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.
Same here... why does _verifyAuthBlockingToken()
expect a session cookie 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.
Same - I copied this test from verifyIdToken, removed it in the latest push
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.
LGTM!
Adding @Xiaoshouzi-gh for visibility |
API Changes