-
Notifications
You must be signed in to change notification settings - Fork 390
feat(auth): Add support for Auth Emulator #1044
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
Conversation
BTW awesome work, will take a detailed pass later once we settle the bigger picture questions |
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 (pending verifyIdToken
change)
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.
Looks mostly good. I've made some suggestions on cleaning up the implementation and improving the test coverage.
@@ -75,9 +75,15 @@ export class FirebaseTokenVerifier { | |||
private publicKeysExpireAt: number; | |||
private readonly shortNameArticle: string; | |||
|
|||
constructor(private clientCertUrl: string, private algorithm: string, | |||
constructor(private clientCertUrl: string, private algorithm: jwt.Algorithm, |
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.
This really ought to accept an options object. But it's ok to do that later.
* @inheritDoc | ||
*/ | ||
public getAccountId(): Promise<string> { | ||
return Promise.resolve('[email protected]'); |
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.
I'm not so sure about this ... could we do something better 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.
cc @yuchenshi
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.
This is probably the best thing we can do
@yuchenshi and @hiranya911 thank you both for your comments, I got the code into a much cleaner state where |
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. This looks pretty good. Just a bunch of nits and cleanup suggestions. Lets use single quotes around string literals everywhere.
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. LGTM 👍
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 with a suggestion.
❤️❤️❤️❤️❤️❤️ |
Thank you for implementing the emulator! Good work! |
* Basic URL replacement and custom tokens * Fix test * Tests actually pass * Mock verifyIdToken * Add another test * Small style change * Small simplification * Significant cleanup of all the useEmulator stuff * Make sure we don't use env var to short-circuit ID Token verification * Make lint pass * Add unit tests that go through the Auth interface * Hiranya nits * Make private method even more private and scary, require env * Make the private method throw * Fix test
Discussion
For Googlers: this design doc explains the behavior changes and security implications.
verifyIdToken
Testing
API Changes
N/A
RELEASE NOTE: Added support for Firebase Auth Emulator.