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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Daniel528
Copy link

@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 ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2025 0:25am

@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

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

Successfully merging this pull request may close these issues.

3 participants