-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(auth): TOTP #7718
base: main
Are you sure you want to change the base?
feat(auth): TOTP #7718
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
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 am sorry - I reviewed this but did not submit the review 🤦 - it looks like a good start (especially at the native level, which is usually the most difficult!) but is not consistent with firebase-js-sdk yet so needs a bit of a re-shape on the API we expose
* Enroll TOTP factor. Provide an optional display name that can be shown to the user. | ||
* The method will ensure the user state is reloaded after successfully enrolling a factor. | ||
*/ | ||
enrollTotp(verificationCode: string, displayName?: string): Promise<void>; |
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.
Interesting - I don't think this is strictly necessary? It is not present in the firebase-js-sdk so we need to change this to make sure we are 100% consistent with them.
What I think we need is to add FactorId.TOTP
to the FactorIdMap
and package the TOTP information in to the MultiFactorAssertion
so we can use it in the enroll
API above
Then this API is not needed
Description
This PR is to add TOTP feature to auth.
Related issues
#7483
Release Summary
Adds TOTP feature to auth.
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Currently working on the tests but would like some feedback before I write a ton of tests if the way I wrote it won't be accepted.
Whats left