Implement #174 to handle additional token errors. #312
Conversation
|
@HofmannZ is attempting to deploy a commit to the Gladly Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report
@@ Coverage Diff @@
## v1.x #312 +/- ##
==========================================
+ Coverage 99.59% 99.80% +0.20%
==========================================
Files 25 25
Lines 495 505 +10
Branches 176 182 +6
==========================================
+ Hits 493 504 +11
+ Misses 2 1 -1
Continue to review full report at Codecov.
|
|
@kmjennison - Ran the failing test locally without my changes, and it fails with the same error. Any idea why that might be? |
|
@HofmannZ I'm not sure. Could you confirm your dependencies match the lockfile? |
|
@kmjennison - Yeah they do. |
There was a problem hiding this comment.
Thanks for the PR! A few changes:
- Handle additional errors to close out this issue: Handle additional errors from verifyIdToken #174
- Add test coverage for new errors
|
@HofmannZ Hmm, ok. I don't have much time to debug at the moment but will try to take a look when I can. |
|
@kmjennison - I will have a look at #174. |
HofmannZ
changed the title
|
@kmjennison - Ready for review! 👀 |
| firebaseUser = null | ||
| break | ||
| default: | ||
| // Otherwise, throw. |
There was a problem hiding this comment.
Not necessarily a change in this PR but wanted to ask your input: I'm wondering if it makes sense to return an unauthenticated user for any error, then call an optional onAuthError callback provided by user for the unexpected errors (ones we don't handle above). My thinking is that it's not particularly easy for developers to catch errors in withAuthUserSSR and in most cases an unauthed user + optional error log is preferable to a 500 error.
There was a problem hiding this comment.
Sounds like a good idea, bit out of scope for this PR. Will add a new PR for that when I find the time for it.
There was a problem hiding this comment.
Thanks, I plan to take this work on soon.
There was a problem hiding this comment.
Implemented this in #366.
As described in #174, there are a couple of scenarios where
verifyIdTokenwas throwing an error, where it could actually be handled more gracefully.This PR:
firebaseandfirebase-admindependencies, andThis PR also brings RFC issue #265 one step closer to being ready for launch 🚀.