feat(auth): add support for multi-factor auth with SMS code #5372
Conversation
|
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/9aLg4UN6wQetv2gYRjXcmCsRtVgv |
There was a problem hiding this comment.
Oh thank you!
I have family visiting this week so I may not be as responsive as normal but I have a few surface-level things I'll try to comment on now
This is a big work, for sure. One general comment I immediately noticed was that it may not have fidelity with firebase-js-sdk which would be a showstopper as noted with links in line comments
A second thing I noticed was that I'm not sure how you're developing this, but there are no jest tests for argument validation, and even though MFA may be hard to set up in our test project for actual MFA validation there are no E2E tests that at least probe the error cases and make sure the code in general is exercised - those are really important for our ability to maintain the code and know future releases are good, so I'll need at least the starts of entries for MFA in the auth jest / e2e area - luckily that's not that hard in fact it makes feature development easier in my experience, to develop that way https://github.com/invertase/react-native-firebase/blob/master/tests/README.md
| @@ -16,6 +16,7 @@ module.exports = { | |||
| version: '16.1.0', | |||
| }, | |||
| }, | |||
| env: { node: true }, | |||
There was a problem hiding this comment.
I'm dubious on need for any formatting changes as part of a feature implementation or bug fix, these should be a separate PR as they'll just confound review of real functionality. Please revert lint changes
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@react-native-firebase/app", | |||
| "version": "12.0.0", | |||
| "version": "12.0.2", | |||
There was a problem hiding this comment.
Versions are managed entirely by lerna and should never be touched in PRs, can you revert all version changes?
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@react-native-firebase/auth", | |||
| "version": "12.0.0", | |||
| "version": "12.0.3", | |||
There was a problem hiding this comment.
Versions are managed entirely by lerna and should never be touched in PRs, can you revert all version changes?
Note that if you need the results of these changes before the PR is approved / merged / released you may pull the "patch-package" artifact from the CI patches check, and incorporate it into your project. That was created to make it easier to close the loop between contributors here and their consuming projects since it's otherwise really difficult to incorporate change from our monorepo structure
| [FIRAuthErrorCodeSecondFactorRequired] = @"multi-factor-auth-required", | ||
| [FIRAuthErrorCodeMissingMultiFactorSession] = @"missing-multi-factor-session", | ||
| [FIRAuthErrorCodeMissingMultiFactorInfo] = @"missing-multi-factor-info", | ||
| [FIRAuthErrorCodeInvalidMultiFactorSession] = @"invalid-multi-factor-session", | ||
| [FIRAuthErrorCodeMultiFactorInfoNotFound] = @"multi-factor-info-not-found", | ||
| [FIRAuthErrorCodeSecondFactorAlreadyEnrolled] = @"second-factor-already-in-use", | ||
| [FIRAuthErrorCodeMaximumSecondFactorCountExceeded] = @"maximum-second-factor-count-exceeded", | ||
| [FIRAuthErrorCodeUnsupportedFirstFactor] = @"unsupported-first-factor", | ||
|
|
There was a problem hiding this comment.
formatting nitpick - do you have a link for these error codes? That would allow me to cross-check for completeness/correctness
| [FIRAuthErrorCodeSecondFactorRequired] = @"multi-factor-auth-required", | |
| [FIRAuthErrorCodeMissingMultiFactorSession] = @"missing-multi-factor-session", | |
| [FIRAuthErrorCodeMissingMultiFactorInfo] = @"missing-multi-factor-info", | |
| [FIRAuthErrorCodeInvalidMultiFactorSession] = @"invalid-multi-factor-session", | |
| [FIRAuthErrorCodeMultiFactorInfoNotFound] = @"multi-factor-info-not-found", | |
| [FIRAuthErrorCodeSecondFactorAlreadyEnrolled] = @"second-factor-already-in-use", | |
| [FIRAuthErrorCodeMaximumSecondFactorCountExceeded] = @"maximum-second-factor-count-exceeded", | |
| [FIRAuthErrorCodeUnsupportedFirstFactor] = @"unsupported-first-factor", | |
| [FIRAuthErrorCodeSecondFactorRequired] = @"multi-factor-auth-required", | |
| [FIRAuthErrorCodeMissingMultiFactorSession] = @"missing-multi-factor-session", | |
| [FIRAuthErrorCodeMissingMultiFactorInfo] = @"missing-multi-factor-info", | |
| [FIRAuthErrorCodeInvalidMultiFactorSession] = @"invalid-multi-factor-session", | |
| [FIRAuthErrorCodeMultiFactorInfoNotFound] = @"multi-factor-info-not-found", | |
| [FIRAuthErrorCodeSecondFactorAlreadyEnrolled] = @"second-factor-already-in-use", | |
| [FIRAuthErrorCodeMaximumSecondFactorCountExceeded] = @"maximum-second-factor-count-exceeded", | |
| [FIRAuthErrorCodeUnsupportedFirstFactor] = @"unsupported-first-factor", |
There was a problem hiding this comment.
https://github.com/firebase/firebase-js-sdk/blob/5ad7ff2ae955c297556223e6cb3ad9d4b897f664/packages-exp/auth-exp/src/core/errors.ts
In various places in docs
https://firebase.google.com/docs/reference/js/firebase.user.MultiFactorUser
https://firebase.google.com/docs/reference/js/firebase.auth.MultiFactorResolver
in ios SDK:
FirbaseAuth/FirAuthErrors.h
| [mfaResolvers removeAllObjects]; | ||
| [credentials removeAllObjects]; |
There was a problem hiding this comment.
nitpick - just for consistency, the rest of the code appears to work on mfaResolvers right after credentials, makes it easier to scan for consistent handling
| [mfaResolvers removeAllObjects]; | |
| [credentials removeAllObjects]; | |
| [credentials removeAllObjects]; | |
| [mfaResolvers removeAllObjects]; |
| FIRUser *user = [FIRAuth authWithApp:firebaseApp].currentUser; | ||
| NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults]; | ||
| NSString *verificationId = [defaults stringForKey:@"authVerificationID"]; | ||
| if (user) | ||
| { |
There was a problem hiding this comment.
same fast return suggestion here
| FIRUser *user = [FIRAuth authWithApp:firebaseApp].currentUser; | |
| NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults]; | |
| NSString *verificationId = [defaults stringForKey:@"authVerificationID"]; | |
| if (user) | |
| { | |
| FIRUser *user = [FIRAuth authWithApp:firebaseApp].currentUser; | |
| if (!user) { | |
| [self promiseNoUser:resolve rejecter:reject isError:YES]; | |
| return; | |
| } | |
| NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults]; | |
| NSString *verificationId = [defaults stringForKey:@"authVerificationID"]; | |
| if (error) { | ||
| [self promiseRejectAuthException:reject error:error]; | ||
| } else { |
There was a problem hiding this comment.
could replicate the "fast return" idea here as well on error conditions. Check for error, reject and return immediately, un-nest the remaining logic so all the un-happy paths are out of the way and we can focus on the happy path and making sure it makes sense
| } else { | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject userInfo:(NSMutableDictionary *) @{ | ||
| @"code": @"unsupported-second-factor", | ||
| @"message": @"This second factor is not supported", | ||
| }]; | ||
| } |
There was a problem hiding this comment.
same fast-error suggestion here - do this error handling first up top and return, then rest of the code is clean mentally from the extra indent / mental load
| return @{ | ||
| @"code": code, | ||
| @"message": message, | ||
| @"nativeErrorMessage": nativeErrorMessage, | ||
| @"authCredential" : authCredentialDict != nil ? (id) authCredentialDict : [NSNull null], | ||
| @"resolver" : mfaResolverDict != nil ? (id) mfaResolverDict : [NSNull null] |
There was a problem hiding this comment.
trialing comma to match prior line
| @"resolver" : mfaResolverDict != nil ? (id) mfaResolverDict : [NSNull null] | |
| @"resolver" : mfaResolverDict != nil ? (id) mfaResolverDict : [NSNull null], |
| * | ||
| */ | ||
|
|
||
| export default class ConfirmationResultMFAEnroll { |
There was a problem hiding this comment.
This one and the SignIn class also need to be in index.d.ts exported as types I believe?
There was a problem hiding this comment.
Also - this will complicate matters a bit but in all ways our types and APIs must match firebase-js-sdk, the stated goal of the project is to be a drop-in replacement for people migrating from firebase-js-sdk to us so we need 100% type/API compliance including when and how errors are thrown unless native means we must do something different
Here's a general hook to the batch of auth classes that should drive the definitions here, the APIs are linked around near this:
https://firebase.google.com/docs/reference/js/firebase.auth#classes
Cases where we might deviate are for instance when we are implementing a synchronous firebase-js-sdk API but it might throw an error, and in our implementation we have to go from JS to native. The only way to throw an error in that case for us since the react-native bridge is async is for us to make the API async in our implementation so we can appropriately bubble the native error up to JS and throw it
There was a problem hiding this comment.
Yes, I agree with you. I actually started that way first and even put all the appropriate types from corresponding firebase-js-sdk here here https://github.com/temaput/react-native-firebase/blob/implement-mfa/packages/auth/lib/index.d.ts, but then I got stuck having no Idea on how to pass opaque multifactor session https://firebase.google.com/docs/reference/ios/firebaseauth/api/reference/Classes#/c:objc(cs)FIRMultiFactorSession
across the bridge and decided to go with a shortcut.
But now, having some experience and your support I believe I could return to that path. I marked the most important questions for me to start going this way.
mikehardy
added
the
Workflow: Needs Review
|
with apologies I want to specifically note you have asked real questions in your description and I have addressed none of them, please have patience and I'll think through things more than surface level as soon as I can 🙏 |
mikehardy
added
the
Workflow: Waiting for User Response
|
@mikehardy @temaput Is there an update on this feature? I'm currently trying to implement SMS-based MFA in react native but I don't see this functionality mentioned in the documentation. |
|
All the development is in the open here, the status of this pr (draft, unmerged, no activity for a while) is the status that exists. If you'd like to take it up you can use it to base work on and I'll happily collaborate with you to get it ready for merge and released |
|
apparently abandoned - happy to collaborate with any one that uses this directly so they can test it and assert it's working - just pull the branch and fix it up and we are in business |
mikehardy
removed
the
Workflow: Needs Review
|
multi-factor released in version 11.3.0! |
Description
Hi guys! Thanks for this wonderful library. I needed to use MFA on my project so I decided to go ahead and add the implementation. I only needed it for IOS so it is IOS only for now.
I can confirm that it works in my project, providing most of the functionality that Firebase provides (including unenrolling and user second factor status reporting).
This is my first experience working with native modules so obviously this implementation is not perfect and I have many doubts on different decisions I had to make. Hence it is only a draft of PR.
Here goes my list of questions if you don't mind:
(/auth/package.json) How should I deal with package versioning? Not touch it? Update all? I decided to bump up the versions of the packages that I updated, but since I had to update the App package (it has the main FirebaseError structure in it) all other packages started complaining about them being outdated.Typescript typings are not done yet. That's not a problem to do for me later.Tests are not written yet. Perhaps you could advise what kind of testing would be crucial here? I mean should I concentrate on e2e with simulator or just some simple unit tests first?Can I modify .eslint?Related issues
Fixes #4166
Release Summary
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter🔥