feat(auth): Add multi-factor support for the sign-in flow #6593

fzuellich · 2022-10-06T16:06:48Z

Description

At Kyan Health we are using react-native-firebase for our mobile apps. We plan to roll out multi-factor authentication to our users soon. After implementing the necessary code on the web (using the original Firebase libraries) we were missing multi-factor support for the apps.

I'm aware that there are a few small todos left in the code. Nonetheless I'd appreciate some early feedback from the maintainers to see how we can get the PR merged. :)

Open questions on our side:

I'm not sure if I'm supposed to add a license header to new files. Let me know and I'm happy to copy the existing header into the newly created files.
I'm running into a lot of trouble with testing the iOS part against the Firebase emulator. I marked these tests with :android:. I have the feeling that this is not properly implemented on Firebase' side. Testing against a real life Firebase project doesn't run into issues with missing multi-factor authentication. I also find it strange the the Firebase emulator prints a different SMS code message for iOS devices (masks the phone number). Any thought or hints how to solve this problem? 😇

Related issues

I'm aware of:

[📚] Does this module support MFA with Firebase? #4166 Adding multi-factor information to the user object IMHO makes only sense if sign-in is supported, otherwise I believe it is not possible to obtain a valid session and the multi-factor information.
feat(auth): add support for multi-factor auth with SMS code #5372 Does provide multi-factor enrolment and sign-in flow. I believe our PR follows the official API closer (https://cloud.google.com/identity-platform/docs/web/mfa), but implements only sign-in and skips the some of the error message conversion.

Release Summary

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- No

Test Plan

e2e tests run
I could verify the multi-factor enrollment and sign-in with a real world Firebase project (iOS)
🔥

vercel · 2022-10-06T16:06:53Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
react-native-firebase	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Oct 24, 2022 at 7:31AM (UTC)

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
react-native-firebase-next	⬜️ Ignored (Inspect)			Oct 24, 2022 at 7:31AM (UTC)

CLAassistant · 2022-10-06T16:06:53Z

All committers have signed the CLA.

docs/auth/phone-auth.md

packages/auth/lib/index.js

docs/auth/mfa-auth.md

mikehardy · 2022-10-14T21:38:37Z

I'm really sorry I haven't gotten to this yet - I really want this functionality, I truly appreciate the PR, and I will collaborate with you to get it merged+released. I appreciate the patience

packages/auth/ios/RNFBAuth/RNFBAuthModule.m

mikehardy

Wow, this looks like excellent work. Finally got a chance to review through it.
I left some mostly-trivial / easy to resolve comments, what do you think?

How is this working for you currently? Does it seem to work? You may not have had a chance to test integrate it yet as the CI runs require approval for first-time PRs so the patch-package patch set is not being generated until I approve.

I just approved a run now which should kick out patches for test integration case that helps

docs/auth/phone-auth.md

mikehardy · 2022-10-17T17:56:36Z

packages/auth/e2e/multiFactor.e2e.js

+ });
+
+ describe('sign-in', function () {
+ it(':android: requires multi-factor auth when enrolled', async function () {


interesting - I'm not familiar with this it(':android: <description>' form of exclusion, is this documented somewhere?

Historically we have been doing the if (<Platform conditional here>) { do the test } else { this.skip() } style I believe

When running the tests e.g. yarn run tests:android:test it outputs the mocha command which uses a grep option:

mocha --config e2e/.mocharc.js --configuration android.emu.debug --grep :ios: --invert --use-custom-logger true e2e

After a quick Google search I couldn't find Detox documentation describing the feature, just some issues:
wix/Detox#712

And a test covering this behaviour: https://github.com/wix/Detox/blob/master/detox/local-cli/test.test.js#L78

Should I move it to the style you mention?

Let me see what the output looks like after the e2e runs, as long as there is still evidence in the output that there is a test but it is skipped (and the total test count / skipped test count registers them), that's the real goal for me. How they are skipped isn't so important

It does not appear to show up in list of skipped tests (it would be an entry with a - like this) so I don't think this works well - it hides them too well. I'd prefer the if-platform/else-skip style

multi-factor ✔ has no multi-factor information if not enrolled ✔ can not reuse session after logout (1035ms) enroll ✔ throws an error for unknown sessions ✔ throws an error for unknown verification code

https://github.com/invertase/react-native-firebase/actions/runs/3271818247/jobs/5385708987#step:23:282

packages/auth/e2e/multiFactor.e2e.js

mikehardy · 2022-10-17T17:57:45Z

packages/auth/e2e/multiFactor.e2e.js

+ .auth()
+ .verifyPhoneNumberForMultiFactor({ phoneNumber: getRandomPhoneNumber(), session });
+ } catch (e) {
+ // TODO fix in code to match web


is this still an active TODO item?

Yes this is one of the TODOs I mentioned. There is at least one more in the Android code.

The todos are sovled now :)

packages/auth/e2e/multiFactor.e2e.js

mikehardy · 2022-10-17T18:07:34Z

Oh - should have written this as nearly the first thing: the test support is absolutely fantastic, really really appreciated. It gives me so much more confidence during review that it does in fact work, knowing it's got e2e tests going with coverage we can see

mikehardy · 2022-10-17T18:11:30Z

Interesting jest failure, referencing appCheck package? Is that a transitive dep of MFA? 🤔


  ● Auth › getMultiFactorResolver › should return the resolver object if its found

    You attempted to use "firebase.app('[DEFAULT]').appCheck" but this module could not be found.

    Ensure you have installed and imported the '@react-native-firebase/app-check' package.

Formatting failure can be fixed with

react-native-firebase/package.json

Line 13 in bf1de6c

 "lint:ios:fix": "clang-format -i --glob=\"packages/**/ios/**/*.{h,cpp,m,mm}\" --style=Google", 

(likewise if there is an android one, just run yarn lint:android locally - possibly a few times until it passes, then commit the result, and for javascript yarn lint:js --fix and/or yarn lint:markdown --write I think - I forget exactly, but basically getting those to not just check but also fix things in place, then commit result

fzuellich · 2022-10-18T08:39:06Z

Very happy to hear you are happy with the PR! :)

Already resolved some of the more trivial comments. I hope I get a chance to look at the rest today, but can't promise it.

How is this working for you currently? Does it seem to work? You may not have had a chance to test integrate it yet as the CI runs require approval for first-time PRs so the patch-package patch set is not being generated until I approve.

I'm linking the package using yalc and played around with it in the iOS simulator (mostly sign-in) yesterday. I'm running the code using our app and against a real Firebase environment. I can also confirm that the Recaptcha part is working. I have a draft for the enroll part lying around somewhere that I'd like to test today. I need to check the Android part is working as well. I hope I get a chance today.

Open todos:

Fix todos in the code
Investigate app-check warning
Possibly change the platform-specific tests to use if instead of :android:

codecov · 2022-10-18T13:14:50Z

Codecov Report

Merging #6593 (1ecb350) into main (fe2bebe) will increase coverage by 17.67%.
The diff coverage is 56.58%.

❗ Current head 1ecb350 differs from pull request most recent head ce15f02. Consider uploading reports for the commit ce15f02 to get more accurate results

@@              Coverage Diff              @@
##               main    #6593       +/-   ##
=============================================
+ Coverage     54.53%   72.20%   +17.67%     
=============================================
  Files           209      115       -94     
  Lines         10399     4754     -5645     
  Branches       1650     1064      -586     
=============================================
- Hits           5670     3432     -2238     
+ Misses         4445     1240     -3205     
+ Partials        284       82      -202

mikehardy · 2022-10-19T15:08:21Z

You may want to rebase this against current main - I've just ingested the major releases firebase-ios-sdk v10 and firebase-android-sdk v31, and on android they did fix a multi-factor issue:

https://firebase.google.com/support/release-notes/android#auth_v21-1-0

The releases themselves are otherwise very low impact (in my opinion), with the only actual breaking change that affects react-native-firebase being a higher minimum iOS target (11) now

I enhanced the android e2e stability as well so CI shouldn't be so finicky.

Other than that, I'll stand by until I hear the "all clear" then I can test through this again. No pressure at all of course, but I am excited to see it merged, whenever it is ready. Cheers

fzuellich · 2022-10-19T15:50:24Z

You may want to rebase this against current main - I've just ingested the major releases firebase-ios-sdk v10 and firebase-android-sdk v31, and on android they did fix a multi-factor issue:

https://firebase.google.com/support/release-notes/android#auth_v21-1-0

Awesome, that explains the broken timestamp when using Android! Do you want me to do a rebase and push-force or rather merge main into this branch?

We have some urgent stuff going right now, so I get less done than I would like.

fzuellich · 2022-10-21T08:17:12Z

The latest commits should fix all todos for this PR. I double checked linting 🤞

Currently I'm unable to integrate the latest changes into our app using yalc. Some classpath issues for Android and iOS. I hope this is resolved when I switch to a patch-package.

Please test through this again. :)

mikehardy · 2022-10-21T12:03:40Z

Huh - never heard of yalc before, interesting! Either way, CI running now, should be a new patch-package set shortly and since I just released (again) last night the patch set will be clean

to match the error message produced by the Web.

fzuellich · 2022-10-23T10:17:57Z

I did some more testing and discovered two smaller issues with the error handling. I pushed the fixes as two commits. I hope it makes it easier to review the changes.

mikehardy · 2022-10-23T16:27:51Z

🤔

❌ /Users/runner/work/react-native-firebase/react-native-firebase/packages/auth/ios/RNFBAuth/RNFBAuthModule.m:786:1: extraneous closing brace ('}')
}

to match the error message produced by the Web.

fzuellich · 2022-10-25T14:47:03Z

Hola @mikehardy, I pushed a fix for that syntax error. If you get a chance can you let the checks run again? :)

mikehardy · 2022-10-25T16:39:38Z

Green CI is a beautiful thing - I'll do one more review pass through here but I imagine this is good to go if it is working for you - the patch-set may be useful in verifying same ?

fzuellich · 2022-10-25T21:22:20Z

I'm quite happy with the iOS part so far (using the patch set). Still having trouble with the Android side due to

Duplicate class com.google.firebase.iid.FirebaseInstanceIdReceiver found in modules jetified-firebase-iid-20.0.1-runtime (com.google.firebase:firebase-iid:20.0.1) and jetified-firebase-messaging-23.1.0-runtime (com.google.firebase:firebase-messaging:23.1.0)

So far the internet wasn't too useful to fix this issue and I secretly hope this will magically solve itself once we have a real package. 🙈

mikehardy · 2022-10-26T00:18:16Z

That one is a pain!

react-native-firebase/packages/messaging/android/build.gradle

Lines 119 to 120 in e6fbf59

 // temporary workaround: https://github.com/firebase/firebase-android-sdk/issues/4206 

 implementation 'com.google.firebase:firebase-iid:21.1.0'

firebase/firebase-android-sdk#4206

fzuellich · 2022-10-26T08:24:13Z

Thanks Mike, your suggestion fixed it! I need to improve my Google powers ;)

I walked through the enrollment and sign-in using Android as well. It works for us 🥳

mikehardy · 2022-10-26T12:53:30Z

Fantastic, so my status understanding is no known issues/ ready for final review+merge release (hopefully) then. Enqueued for me as such

fzuellich · 2022-10-26T13:12:05Z

Sounds good. I'm sure we'll be the first to open a PR if we find a bug. 😇 Do you have an ETA that I can communicate to management?

mikehardy

Re-scanned it all for final review ✅
Tested e2e locally after pulling current main + rebase (just to be sure)

ios ✅
android ✅

Merging now, will release in just a moment so should be available as 16.3.0 in a few minutes.

You can communicate to management that the quality of the PR and attention to detail on testing gives me a really high level of confidence, making quick review possible and leading to me shipping it immediately despite the size of the PR, how's that vs an ETA ;-)?. This is a fantastic addition to the module, I really appreciate it and I'm sure others hoping for it will be thrilled as well. Happy to collaborate on any followups as needed if (or when, it's a big feature...) something comes up. Cheers

fzuellich · 2022-10-27T07:01:31Z

That sounds awesome. Thank you so much for the collaboration!

mikehardy · 2022-10-28T00:38:16Z

@fzuellich not sure if this is relevant for you or not, but saw it in the release notes for just-released 10.1.0 of firebase-ios-sdk and thought you might be interested, it just released: firebase/firebase-ios-sdk#10296

I'm releasing the SDK update packaged into a version here shortly so that fix (among others) will be generally available here

fzuellich changed the title ~~Add multi-factor support for the sign-in flow~~ feat(auth): Add multi-factor support for the sign-in flow Oct 6, 2022

vercel bot deployed to Preview – react-native-firebase October 6, 2022 16:13 View deployment

fzuellich commented Oct 7, 2022

View reviewed changes

docs/auth/phone-auth.md Outdated Show resolved Hide resolved

fzuellich commented Oct 7, 2022

View reviewed changes

packages/auth/lib/index.js Outdated Show resolved Hide resolved

fzuellich commented Oct 7, 2022

View reviewed changes

packages/auth/lib/index.js Outdated Show resolved Hide resolved

fzuellich force-pushed the add-multi-factor-support branch from c7ba289 to fdf92f9 Compare October 11, 2022 16:07

vercel bot deployed to Preview – react-native-firebase October 11, 2022 16:12 View deployment

fzuellich commented Oct 11, 2022

View reviewed changes

docs/auth/mfa-auth.md Outdated Show resolved Hide resolved

fzuellich force-pushed the add-multi-factor-support branch from fdf92f9 to 5a6f5f5 Compare October 14, 2022 09:01

vercel bot deployed to Preview – react-native-firebase October 14, 2022 09:06 View deployment

fzuellich force-pushed the add-multi-factor-support branch from 5a6f5f5 to 0861ac4 Compare October 14, 2022 21:00

vercel bot deployed to Preview – react-native-firebase October 14, 2022 21:06 View deployment

fzuellich marked this pull request as ready for review October 14, 2022 21:25

vercel bot deployed to Preview – react-native-firebase October 14, 2022 21:30 View deployment

fzuellich commented Oct 17, 2022

View reviewed changes

packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved

vercel bot deployed to Preview – react-native-firebase October 17, 2022 11:10 View deployment

mikehardy mentioned this pull request Oct 17, 2022

Allow for static sms code firebase/firebase-tools#4290

Open

mikehardy requested changes Oct 17, 2022

View reviewed changes

mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Oct 17, 2022

vercel bot deployed to Preview – react-native-firebase October 18, 2022 08:37 View deployment

vercel bot deployed to Preview – react-native-firebase October 19, 2022 15:55 View deployment

fzuellich added 3 commits October 21, 2022 09:25

feat(auth): multi-factor enroll feature for Android and e2e-tests

b5f2e5a

feat(auth): Implement multi-factor enrollment for iOS

452f57f

fix(tests): More robust matchers for invalid-verification-code error

97cfa44

fzuellich force-pushed the add-multi-factor-support branch from 5595da3 to 97cfa44 Compare October 21, 2022 07:58

vercel bot deployed to Preview – react-native-firebase October 21, 2022 08:03 View deployment

Fix error message for invalid phone numbers

e38b5c3

to match the error message produced by the Web.

vercel bot deployed to Preview – react-native-firebase October 23, 2022 10:22 View deployment

Add test and fix error message for unknown multi-factor hint

ce15f02

to match the error message produced by the Web.

fzuellich force-pushed the add-multi-factor-support branch from 3e7d5fc to ce15f02 Compare October 24, 2022 07:25

vercel bot deployed to Preview – react-native-firebase October 24, 2022 07:30 View deployment

mikehardy self-requested a review October 25, 2022 15:10

mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Oct 25, 2022

mikehardy approved these changes Oct 26, 2022

View reviewed changes

mikehardy merged commit 3c64bf5 into invertase:main Oct 26, 2022

mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Oct 26, 2022

fzuellich deleted the add-multi-factor-support branch October 26, 2022 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): Add multi-factor support for the sign-in flow #6593

feat(auth): Add multi-factor support for the sign-in flow #6593

fzuellich commented Oct 6, 2022 •

edited

Loading

vercel bot commented Oct 6, 2022 •

edited

Loading

CLAassistant commented Oct 6, 2022 •

edited

Loading

mikehardy commented Oct 14, 2022

mikehardy left a comment

mikehardy Oct 17, 2022

fzuellich Oct 18, 2022

mikehardy Oct 18, 2022

mikehardy Oct 18, 2022 •

edited

Loading

fzuellich Oct 19, 2022

mikehardy Oct 17, 2022

fzuellich Oct 18, 2022

fzuellich Oct 24, 2022

mikehardy commented Oct 17, 2022

mikehardy commented Oct 17, 2022

fzuellich commented Oct 18, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading

mikehardy commented Oct 19, 2022

fzuellich commented Oct 19, 2022

fzuellich commented Oct 21, 2022

mikehardy commented Oct 21, 2022

fzuellich commented Oct 23, 2022

mikehardy commented Oct 23, 2022

fzuellich commented Oct 25, 2022

mikehardy commented Oct 25, 2022

fzuellich commented Oct 25, 2022

mikehardy commented Oct 26, 2022

fzuellich commented Oct 26, 2022

mikehardy commented Oct 26, 2022 •

edited

Loading

fzuellich commented Oct 26, 2022

mikehardy left a comment

fzuellich commented Oct 27, 2022

mikehardy commented Oct 28, 2022

feat(auth): Add multi-factor support for the sign-in flow #6593

feat(auth): Add multi-factor support for the sign-in flow #6593

Conversation

fzuellich commented Oct 6, 2022 • edited Loading

Description

Open questions on our side:

Related issues

Release Summary

Checklist

Test Plan

vercel bot commented Oct 6, 2022 • edited Loading

CLAassistant commented Oct 6, 2022 • edited Loading

mikehardy commented Oct 14, 2022

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Oct 17, 2022

Choose a reason for hiding this comment

fzuellich Oct 18, 2022

Choose a reason for hiding this comment

mikehardy Oct 18, 2022

Choose a reason for hiding this comment

mikehardy Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

fzuellich Oct 19, 2022

Choose a reason for hiding this comment

mikehardy Oct 17, 2022

Choose a reason for hiding this comment

fzuellich Oct 18, 2022

Choose a reason for hiding this comment

fzuellich Oct 24, 2022

Choose a reason for hiding this comment

mikehardy commented Oct 17, 2022

mikehardy commented Oct 17, 2022

fzuellich commented Oct 18, 2022 • edited Loading

codecov bot commented Oct 18, 2022 • edited Loading

Codecov Report

mikehardy commented Oct 19, 2022

fzuellich commented Oct 19, 2022

fzuellich commented Oct 21, 2022

mikehardy commented Oct 21, 2022

fzuellich commented Oct 23, 2022

mikehardy commented Oct 23, 2022

fzuellich commented Oct 25, 2022

mikehardy commented Oct 25, 2022

fzuellich commented Oct 25, 2022

mikehardy commented Oct 26, 2022

fzuellich commented Oct 26, 2022

mikehardy commented Oct 26, 2022 • edited Loading

fzuellich commented Oct 26, 2022

mikehardy left a comment

Choose a reason for hiding this comment

fzuellich commented Oct 27, 2022

mikehardy commented Oct 28, 2022

fzuellich commented Oct 6, 2022 •

edited

Loading

vercel bot commented Oct 6, 2022 •

edited

Loading

CLAassistant commented Oct 6, 2022 •

edited

Loading

mikehardy Oct 18, 2022 •

edited

Loading

fzuellich commented Oct 18, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading

mikehardy commented Oct 26, 2022 •

edited

Loading