(chore): Add JWT Decoder and Signature Verifier by lahirumaramba · Pull Request #1204 · firebase/firebase-admin-node

lahirumaramba · 2021-03-21T21:37:16Z

Refactoring verify token take 2
Add JWT Decoder class

hiranya911

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

src/utils/jwt-decoder.ts

hiranya911

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.

src/utils/jwt-decoder.ts

src/utils/jwt-signature-verifier.ts

src/auth/token-verifier.ts

hiranya911

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

src/utils/jwt.ts

src/auth/token-verifier.ts

hiranya911 · 2021-03-24T22:51:08Z

src/auth/token-verifier.ts

-    const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt);
-    if (publicKeysExist && publicKeysStillValid) {
-      return Promise.resolve(this.publicKeys);
+    if (!(error instanceof JwtError)) {


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

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

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

src/utils/jwt.ts

lahirumaramba

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!

src/auth/token-verifier.ts

src/utils/jwt.ts

src/auth/token-verifier.ts

lahirumaramba · 2021-03-25T00:58:53Z

src/auth/token-verifier.ts

-    const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt);
-    if (publicKeysExist && publicKeysStillValid) {
-      return Promise.resolve(this.publicKeys);
+    if (!(error instanceof JwtError)) {


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

hiranya911

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

src/utils/jwt.ts

src/auth/token-verifier.ts

hiranya911 · 2021-03-25T21:06:49Z

src/auth/token-verifier.ts

-    const publicKeysStillValid = (publicKeysExpiredExists && Date.now() < this.publicKeysExpireAt);
-    if (publicKeysExist && publicKeysStillValid) {
-      return Promise.resolve(this.publicKeys);
+    if (!(error instanceof JwtError)) {


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

src/auth/token-verifier.ts

lahirumaramba requested a review from hiranya911 March 21, 2021 21:39

hiranya911 suggested changes Mar 22, 2021

View reviewed changes

src/utils/jwt-decoder.ts Outdated Show resolved Hide resolved

src/utils/jwt-decoder.ts Outdated Show resolved Hide resolved

src/utils/jwt-decoder.ts Outdated Show resolved Hide resolved

hiranya911 assigned lahirumaramba Mar 22, 2021

lahirumaramba force-pushed the lm-verify-jwt-take2 branch from 0093f57 to e118e6a Compare March 23, 2021 23:14

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Mar 23, 2021

hiranya911 suggested changes Mar 24, 2021

View reviewed changes

lahirumaramba commented Mar 24, 2021

View reviewed changes

src/auth/token-verifier.ts Outdated Show resolved Hide resolved

lahirumaramba marked this pull request as ready for review March 24, 2021 22:05

lahirumaramba force-pushed the lm-verify-jwt-take2 branch from 83852f5 to 5a1b28c Compare March 24, 2021 22:12

lahirumaramba changed the title ~~(chore): Add JWT Decoder~~ (chore): Add JWT Decoder and Signature Verifiers Mar 24, 2021

lahirumaramba changed the title ~~(chore): Add JWT Decoder and Signature Verifiers~~ (chore): Add JWT Decoder and Signature Verifier Mar 24, 2021

hiranya911 suggested changes Mar 24, 2021

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Mar 24, 2021

lahirumaramba mentioned this pull request Mar 24, 2021

chore: Move token verification logic to util #1189

Closed

hiranya911 reviewed Mar 24, 2021

View reviewed changes

src/utils/jwt.ts Outdated Show resolved Hide resolved

lahirumaramba commented Mar 25, 2021

View reviewed changes

lahirumaramba force-pushed the lm-verify-jwt-take2 branch from 656276d to 8848ac6 Compare March 25, 2021 01:15

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Mar 25, 2021

hiranya911 approved these changes Mar 25, 2021

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Mar 25, 2021

lahirumaramba added 5 commits March 25, 2021 20:52

(chore): Add JWT Decoder

a539e68

Add signature verifier and key fetcher abstractions

bc5bb6e

Move the signature verifier to utils/jwt and other PR fixes

5983c19

Fix http import

41d42df

PR fixes

04b2c54

lahirumaramba force-pushed the lm-verify-jwt-take2 branch from 05e56e4 to 752de13 Compare March 26, 2021 00:52

davidhin mentioned this pull request Jul 6, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#682

Open

This was referenced Jul 17, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#685

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#687

Open

This was referenced Aug 10, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#697

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#700

Open

This was referenced Aug 28, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#707

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#709

Open

This was referenced Sep 5, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#711

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#712

Open

This was referenced Oct 26, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#729

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#731

Open

davidhin mentioned this pull request Nov 9, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#732

Open

Conversation

lahirumaramba commented Mar 21, 2021

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

lahirumaramba Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

hiranya911 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lahirumaramba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lahirumaramba Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants