Skip to content

(chore): Add JWT Decoder and Signature Verifier - #1204

Merged
lahirumaramba merged 8 commits into
masterfrom
lm-verify-jwt-take2
Mar 30, 2021
Merged

(chore): Add JWT Decoder and Signature Verifier#1204
lahirumaramba merged 8 commits into
masterfrom
lm-verify-jwt-take2

Conversation

@lahirumaramba

@lahirumarambalahirumaramba commented Mar 21, 2021

Copy link
Copy Markdown
Member
  • Refactoring verify token take 2
  • Add JWT Decoder class

hiranya911
hiranya911 suggested changes Mar 22, 2021
View reviewed changes

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a bit more work in terms of getting the abstractions right. I've proposed some ideas. PTAL.

Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-decoder.ts Outdated
hiranya911
hiranya911 suggested changes Mar 24, 2021
View reviewed changes

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-decoder.ts Outdated
Comment threadsrc/utils/jwt-signature-verifier.ts Outdated
Comment threadsrc/utils/jwt-signature-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
lahirumaramba
lahirumaramba commented Mar 24, 2021
View reviewed changes
Comment threadsrc/auth/token-verifier.ts Outdated
@lahirumaramba
lahirumaramba marked this pull request as ready for review March 24, 2021 22:05
@lahirumarambalahirumaramba changed the title (chore): Add JWT Decoder(chore): Add JWT Decoder and Signature VerifiersMar 24, 2021
@lahirumarambalahirumaramba changed the title (chore): Add JWT Decoder and Signature Verifiers(chore): Add JWT Decoder and Signature VerifierMar 24, 2021
hiranya911
hiranya911 suggested changes Mar 24, 2021
View reviewed changes

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all the feedback. Looks pretty good! Few more things to revise and we are good to go.

Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt);
if (publicKeysExist && publicKeysStillValid) {
return Promise.resolve(this.publicKeys);
if (!(error instanceof JwtError)) {

@hiranya911hiranya911Mar 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious as to what other error conditions can pop up here. Can we guarantee that this API only ever sees JwtError instances?

@lahirumarambalahirumarambaMar 25, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We throw HttpError from KeyFetcher (if the fetcher is used directly without the getKey callback) and Error types for value errors.

@hiranya911hiranya911Mar 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't call that API directly, I think that case can never occur right? We will only see JwtError here.

Comment threadsrc/utils/jwt.ts
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
@lahirumarambalahirumaramba mentioned this pull request Mar 24, 2021
Closed
hiranya911
hiranya911 reviewed Mar 24, 2021
View reviewed changes
Comment threadsrc/utils/jwt.ts Outdated
lahirumaramba
lahirumaramba commented Mar 25, 2021
View reviewed changes

@lahirumarambalahirumaramba left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt);
if (publicKeysExist && publicKeysStillValid) {
return Promise.resolve(this.publicKeys);
if (!(error instanceof JwtError)) {

@lahirumarambalahirumarambaMar 25, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We throw HttpError from KeyFetcher (if the fetcher is used directly without the getKey callback) and Error types for value errors.

hiranya911
hiranya911 approved these changes Mar 25, 2021
View reviewed changes

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm at an LGTM with a few open comments/suggestions.

Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/utils/jwt.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt);
if (publicKeysExist && publicKeysStillValid) {
return Promise.resolve(this.publicKeys);
if (!(error instanceof JwtError)) {

@hiranya911hiranya911Mar 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't call that API directly, I think that case can never occur right? We will only see JwtError here.

Comment threadsrc/auth/token-verifier.ts Outdated
Comment threadsrc/auth/token-verifier.ts Outdated
This was referenced Jun 20, 2024
Open
Open
Open
Open
@davidhindavidhin mentioned this pull request Jul 6, 2024
Open
This was referenced Jul 17, 2024
Open
Open
This was referenced Jul 27, 2024
Open
Open
Open
Open
This was referenced Aug 10, 2024
Open
Open
This was referenced Aug 17, 2024
Open
Open
Open
Open
Open
This was referenced Aug 28, 2024
Open
Open
This was referenced Sep 5, 2024
Open
Open
This was referenced Sep 24, 2024
Open
Open
Open
Open
Open
This was referenced Oct 26, 2024
Open
Open
@davidhindavidhin mentioned this pull request Nov 9, 2024
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@hiranya911hiranya911hiranya911 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lahirumarambalahirumaramba

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lahirumaramba@hiranya911