(chore): Add JWT Decoder and Signature Verifier#1204
Conversation
lahirumaramba
commented
Mar 21, 2021
- Refactoring verify token take 2
- Add JWT Decoder class
hiranya911
left a comment
There was a problem hiding this comment.
I think this needs a bit more work in terms of getting the abstractions right. I've proposed some ideas. PTAL.
0093f57 to
e118e6a
Compare
hiranya911
left a comment
There was a problem hiding this comment.
Overall approach and design is correct. I found a bunch of nits that can be cleaned up. I haven't looked at all the error handling bits yet. I'll get to that in the next round.
83852f5 to
5a1b28c
Compare
hiranya911
left a comment
There was a problem hiding this comment.
Thanks for addressing all the feedback. Looks pretty good! Few more things to revise and we are good to go.
| const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt); | ||
| if (publicKeysExist && publicKeysStillValid) { | ||
| return Promise.resolve(this.publicKeys); | ||
| if (!(error instanceof JwtError)) { |
There was a problem hiding this comment.
Just curious as to what other error conditions can pop up here. Can we guarantee that this API only ever sees JwtError instances?
There was a problem hiding this comment.
We throw HttpError from KeyFetcher (if the fetcher is used directly without the getKey callback) and Error types for value errors.
There was a problem hiding this comment.
Since we don't call that API directly, I think that case can never occur right? We will only see JwtError here.
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you for the feedback!!
Updated the PR.
I removed the algorithm parameter from the API and updated the unit tests to reflect that change.
I will follow up with another update to add unit tests for the new components. Thanks!
| const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt); | ||
| if (publicKeysExist && publicKeysStillValid) { | ||
| return Promise.resolve(this.publicKeys); | ||
| if (!(error instanceof JwtError)) { |
There was a problem hiding this comment.
We throw HttpError from KeyFetcher (if the fetcher is used directly without the getKey callback) and Error types for value errors.
656276d to
8848ac6
Compare
hiranya911
left a comment
There was a problem hiding this comment.
I'm at an LGTM with a few open comments/suggestions.
| const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt); | ||
| if (publicKeysExist && publicKeysStillValid) { | ||
| return Promise.resolve(this.publicKeys); | ||
| if (!(error instanceof JwtError)) { |
There was a problem hiding this comment.
Since we don't call that API directly, I think that case can never occur right? We will only see JwtError here.
05e56e4 to
752de13
Compare