-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
refactor: filter out any multifactor besides phone #8645
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
f5b63e5
to
7ee7223
Compare
7ee7223
to
080d44b
Compare
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.
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
0cfd705
to
b1f3392
Compare
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 I'm unable to replicate the failing test case on my local environment. Initially when running
Setting 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 :) |
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 |
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 |
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. |
releasing now should be up on npmjs.com shortly barring publish problems, cheers! |
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
inreact-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 throughsignInWithCustomToken
.I have not encountered this crashing in Android when testing. The comparable java function
multiFactorInfoToMap
inReactNativeFirebaseAuthModule.java
does not crash.To resolve this issue I've rewritten the
convertMultiFactorData
function inRNFBAuthModule.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 anyFIRMultiFactorInfo
hints which do not match theFIRPhoneMultiFactorInfo
class. The implementation is more similar to the java implementation ofmultiFactorInfoToMap
.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
Android
iOS
Other
(macOS, web)e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter🔥