Skip to content

refactor: filter out any multifactor besides phone #8645

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

Merged

Conversation

Daniel528
Copy link
Contributor

@Daniel528 Daniel528 commented Aug 8, 2025

Description

Our project has a web dashboard (implementing Firebase JS SDK) and react native app (using react-native-firebase) which share a Firebase project. We are planning on supporting MFA through TOTP & SMS. This is possible with the JS SDK, TOTP is currently unsupported in react-native-firebase. To get around this, we are planning on having the react native app open an external browser window to the web dashboard and have users authenticate through this. Once authenticated in the external browser, a custom token is passed back to the app and the function signInWithCustomToken in react-native-firebase is used to authenticate the user.

We ran into this error when a user account who adds a TOTP MFA method on the web dashboard then attempts to log into the react-native app will encounter that crash on iOS instead of receiving a firebase auth error with code auth/multi-factor-auth-required. This crash also occurs when the user attempts to authenticate through signInWithCustomToken.

I have not encountered this crashing in Android when testing. The comparable java function multiFactorInfoToMap in ReactNativeFirebaseAuthModule.java does not crash.

To resolve this issue I've rewritten the convertMultiFactorData function in RNFBAuthModule.m so it no longer references the .phoneNumber property of a hint which does not have that field. Because only SMS is supported, I've opted to completely filter out any FIRMultiFactorInfo hints which do not match the FIRPhoneMultiFactorInfo class. The implementation is more similar to the java implementation of multiFactorInfoToMap.

I've also removed the getJSFactorId function which seemed ineffective and was no longer referenced after this refactor.

I'm aware of this PR which also resolves the issue I'm trying to solve. From what I can see though that PR may take a bit longer to resolve and dependant on Android Emulator testing. If that is the case then this MR can basically be a small bridge to resolve the crash immediately at hand before working on the larger TOTP implementation.

Related issues

#8601 (comment)

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
    • Other (macOS, web)
  • 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


Think react-native-firebase is great? Please consider supporting the project with any of the below:

🔥

Copy link

vercel bot commented Aug 8, 2025

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

Project Deployment Preview Comments Updated (UTC)
react-native-firebase Ready Preview 💬 Add feedback Aug 12, 2025 0:57am

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed and well-reasoned description on the PR.

I saw that crash come in the issue tracker and it was concerning - I not like having any known crash bugs in the repo if I can help it, and I do not want to block a crash fix on a future PR that needs a bit more time to develop, so I'd love a fix for the issue in general

This fix in particular looks good - certainly good enough - but also ready for future factor types. I see no reason not to merge it pending CI

I can do a release with this as a fix once merged, in the meantime the patch-package CI task should generate a patchset that contains only approved + merged-to-main items plus this, since last release. It can be relied on if you aren't already running with this change locally

Thanks a bunch for the PR

@Daniel528
Copy link
Contributor Author

Daniel528 commented Aug 12, 2025

Hey @mikehardy, (or anyone else with the domain knowledge)

Do you have any ideas on what I can do to resolve the failing Android e2e tests? Considering the iOS exclusive auth module changes in this PR, I'm unsure why Android tests in the database module would be failing while (if I read the logs correctly) all tests inmultiFactor.e2e.js are running successfully.

I'm unable to replicate the failing test case on my local environment. Initially when running yarn tests:android:test locally they'd exit prematurely with what I assume to be an internal jet error based on this output:

[🟨] Jet client disconnected - for no particular reason (code = 1006).
[🟥] Exiting after an abnormal disconnect.

Setting exitOnError: false in .jetrc.js though allows the whole test suite to run and all tests pass. This is my first time using jet. I'm hoping the suite passing entirely with exitOnError: false is satisfactory or if it crashing with exitOnError: true is related the GitHub actions failing tests and hopefully something I can resolve.

At this stage I'm uncertain and having my local and the logs from GitHub actions aligned would be handy for moving forward. Any help would be very nice, thank you :)

@mikehardy
Copy link
Collaborator

I've strugged with that test before, the test is flaky, it is not the fault of this PR

Disabling the jet exit on error means that the tests will run, however any failures encountered in the test suite will be ignored so all CI runs will be potentially false positive. The solution is for the test that's failing to be fixed so it is not flaky, or for it to be disabled in CI so it stops triggering false negatives

I'll get this merged - obviously if iOS passes on an iOS-only changeset, an android CI failure is not important

I've re-run CI since you pushed a merge commit - if iOS goes green this is merge-able, no worries

@Daniel528
Copy link
Contributor Author

Thanks for re-running the tests. The debug iOS tests have passed but release iOS tests have failed.

Both release & debug pass completely when I run them locally.

The failing test is in the app-check package rather than the auth. Is this likely to be related to the contents of the PR or more likely another flakey test?

@mikehardy
Copy link
Collaborator

The failing test is in the app-check package rather than the auth. Is this likely to be related to the contents of the PR or more likely another flakey test?

Flaky - app-check in particular puts rate limits on token exchange events in the cloud so one of our CI failure modes is for one iOS run to work but one to fail on app-check if they run concurrently, and then for the failing one to succeed if re-run separately. It wasn't flaky prior to all the modular API implementation but now all APIs are effectively run twice, on 3 platforms (android, ios release, ios debug), which may be happening for multiple PRs or main merges at once, which at the end of it all has resulted in this popping up more often. CI stability is always within reach but never quite there it seems 😅

No worries though, this will be merged and released shortly - it's next on the list. And no flakes are the fault of the PR obviously.

@mikehardy mikehardy merged commit 90b6d98 into invertase:main Aug 12, 2025
17 of 19 checks passed
@mikehardy
Copy link
Collaborator

releasing now should be up on npmjs.com shortly barring publish problems, cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants