Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(auth): support TOTP enroll and unenroll #8621

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
mikehardy merged 10 commits into invertase:main from jamespb97:totp-mfa-unenroll
Aug 29, 2025

Conversation

Copy link
Contributor

@jamespb97 jamespb97 commented Jul 23, 2025
edited
Loading

Description

The aim of these changes is to enable TOTP MFA and the ability to un-enroll from either SMS or TOTP MFA.

The motivation behind doing this was to migrate away from Firebase JS SDK implementation on our existing Expo application which extensively uses multi-factor authentication.

This is my first time ever changing native code like this. My workflow at the moment for making these changes have simply been editing the native modules in the respective code editors (Xcode and Android Studio), then using patch-package and building new development builds each time and testing live against our development environment, so nothing has been emulated.

I tried to link my forked repo locally using npm/yarn link and build off that instead but I couldn't get it to work so I had to go the patch-package route and copy across all of my changes to my fork manually. If you're aware of a better way to do this please let me know!

I am aware that there is a blocking issue (firebase/firebase-tools#6224) which prevents TOTP being tested within emulation, so I guess proper tests cannot be written just yet for this feature (unsure about unenroll feature).

Related issues

#7483
Abandoned related PR: #7718

Release Summary

Add support for enrolling TOTP multi-factor authentication, authentication using TOTP and un-enrolling multi-factor authentication.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
  • 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

I currently only have this working in our product application which won't be possible to post videos or screenshots of on a public PR, but I can create a minimum repro app soon enough (probably in my free time over the weekend) and post videos and screenshots from that instead.

Update:
As promised, here is a demo of the changes in action along with a demo repo: https://github.com/jamespb97/firebase-totp-demo
To run just change the app.json accordingly and copy across your firebase service files and it should all work.

Android:

Android.Demo.mp4

iOS:

iOS.demo.mp4

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

🔥 🔥 🔥

kamilwezgowiec and Nantris reacted with thumbs up emoji
Copy link

vercel bot commented Jul 23, 2025
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
react-native-firebase Ready Ready Preview Comment Aug 29, 2025 2:08am

Copy link

CLAassistant commented Jul 23, 2025
edited
Loading

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

I'm not aware of a better way to do this given we don't have TOTP support in the emulator, your patch-package based development workflow sounds both a) really painful and objectively awful and also b) like the best way

Sorry for the a) painful + awful part, I wish there was a better way

That said, this is incredible if you've got this working with an app that uses MFA and TOTP so you know it's working on both platforms - for the avoidance of doubt, you confirm that is the case? it is tested and working in your app on android and iOS ? If so, this is exciting

Copy link
Contributor Author

@mikehardy I appreciate the feedback re my workflow 😆

For the proof, yes, I was meant to whip up a little demo app which I just didn't get time to do over the weekend. I'll see what I can do, hopefully, this week.

I am just about to deploy our app to our production environment with this patch as it has all been going very well in testing. Any fixes or changes that might crop up I am making sure to push back into this PR.

mikehardy reacted with hooray emoji

Copy link
Contributor

Hey @mikehardy I've seen there's the firebase emulator blocking issue which doesn't seem to have any movement from the emulator team.

Is a TOTP implementation for this library at all possible to merge into main without e2e testing? If the maintainers are comfortable having it implemented without the full automated test suite and with a heavy caveat for anyone who chooses to use it, I'm happy to lend some time to get this PR polished and merged to main.

If it's not a go without those tests though then all good.

Copy link
Collaborator

I've got a different way to verify it I think and will be trying an experiment to verifiy if it will work or not shortly. The PR to implement it can go in either way though. Can't really have a regression on something that didn't work before, no reason to reject a community PR based on that...

Daniel528 reacted with thumbs up emoji

Copy link
Contributor Author

@mikehardy I have updated the description with demos as promised. Let me know what you think

Copy link
Contributor

@jamespb97 A PR I've authored recently will conflict with this PR. I'm happy to manage the conflicts if you bring this fork up to date with main. Can make a branch and get you to review it before merging back the PR branch.

Copy link

Great contribution @jamespb97!
This feature is something very important for a project I'm working on, there's any updates? Estimated time when this will be available in a new version of the library? Thanks in advance

jamespb97 reacted with heart emoji

Copy link
Contributor Author

@jamespb97 A PR I've authored recently will conflict with this PR. I'm happy to manage the conflicts if you bring this fork up to date with main. Can make a branch and get you to review it before merging back the PR branch.

No problem, I’m away the next few days but when I get a moment I’ll sort it out 👍

Daniel528 reacted with heart emoji

Copy link
Contributor Author

@Daniel528 Merge "should" work but I will be updating on testing by end of the day

Copy link
Contributor Author

@Daniel528 Now it works 👍

Daniel528 reacted with heart emoji

Copy link
Contributor

Yer groovy, good one line change to catch TOTP. From what I can see there's no unique properties associated with TOTP we need to append to the factor dictionary like the phone requires.

Copy link
Contributor Author

Yer groovy, good one line change to catch TOTP. From what I can see there's no unique properties associated with TOTP we need to append to the factor dictionary like the phone requires.

Agreed. Since this library just needs parity with the web SDK, there seems to be no extra details with TOTP, as per the TotpMultiFactorInfo type definition in web sdk.

Copy link
Collaborator

mikehardy commented Aug 27, 2025
edited
Loading

My attempt at creating a more regular "manual verification" pathway for testing that isn't supported by automated e2e testing / firebase simulator appears to work

With that setup I can use our existing e2e test app (with all it's configuration setup as needed for TOTP verification...) to make a manual TOTP testing panel and get this PR merged with confidence

I'm going to pull this branch locally and try to do that manual test panel in the e2e app, and hopefully get this thing merged at long last!

Copy link
Collaborator

I backed out the merge and squashed it down to one feature commit, so I could cleanly rebase my testing PR merge into here, I'll work on the test app panel today and repush when I think I've got it

I'm reasonably certain the functionality works great - which is amazing, and thank you - just need to make sure it's maintainable over course of years and years so I've got to have a way to easily check it in the future

@mikehardy mikehardy changed the title (削除) feat(auth): TOTP and unenroll MFA (削除ここまで) (追記) feat(auth): support TOTP enroll and unenroll (追記ここまで) Aug 27, 2025
Copy link
Collaborator

Going to shop it internally for review, but the first thing I can think of is that the TotpSecret class doesn't seem to have typescript types, though I may have missed them. The JS works though, so if they are missing it will be an easy fix

My bad I must have missed that when copying across to the fork but I do have the class defined in my patch. I can copy across if you'd like if you haven't already done it

I haven't yet and will be out of the office next few hours, if you could add it that would be great. Likely need to reset hard on the branch from github, ive groomed the commits via rebase merge and force push...

jamespb97 reacted with thumbs up emoji

Copy link
Collaborator

mikehardy commented Aug 28, 2025
edited
Loading

thoughts rattling around my head while out and about for pre-merge:

  • put co-authored-by git commit attribution on the test app commit
  • check copyright headers everywhere (test app should have them as well)
  • carefully compare all typing with upstream to make sure
  • check docs site typescript generation to make sure typing is good
  • possibly implement native-only openInOTPApp - supported ios/android, not web but that's one of the reasons to use native vs web sdk...
  • verify Other platform support, testing was done ios/android so far, not Other
  • triple check flow and API calls in manual test vs web implementation guide for API fidelity / compatibility

More of a checklist for verification then lots of work but I forget checklists if I don't write them down

James Bateman and others added 7 commits August 28, 2025 13:43
----
Co-Authored-By: Mike Hardy <github@mikehardy.net>
that way if other tests need to use cloud database vs emulator,
it is still possible until this test is run
took demo app from PR and ingested it here as local test
- added ability to sign up a new email/pass, not just login
- added ability to detect unverified email, send verify email, and reload user
- added QR code display for TOTP enroll flow
- added native TOTP app open demonstration
----
Co-Authored-By: James Bateman <jpbateman97@gmail.com>
Copy link
Collaborator

mikehardy commented Aug 29, 2025
edited
Loading

underestimated the complexity of adding support for the web / Other platform through our wrappers but got that working and have done all the typing and docs changes needed.

I did add openInOtpApp :-)

Just re-reading one more time then I'll merge and release.

This is an absolutely huge feature for react-native-firebase, I'm so pleased to merge this. Thanks for posting it up - please @ tag me if there is a need for any followups, I drown in more generic notifications and don't want to miss any related to this.

@mikehardy mikehardy merged commit af15088 into invertase:main Aug 29, 2025
17 of 22 checks passed
Copy link
Collaborator

Released as 23.2.0

https://github.com/invertase/react-native-firebase/blob/main/CHANGELOG.md#2320-2025年08月29日

react-native-firebase finally has TOTP. Wow, we're all grown up for auth at long last

JFGHT, npfedwards, and Daniel528 reacted with hooray emoji jamespb97 reacted with rocket emoji

Copy link
Contributor Author

Wow! Very speedy work @mikehardy! Thanks so much for getting this over the line, you're a legend!

mikehardy reacted with heart emoji

Copy link

JFGHT commented Aug 29, 2025

Been observing this PR and I can only say; thank you @mikehardy and @jamespb97 for the hard work.

jamespb97, mikehardy, Daniel528, and Nantris reacted with heart emoji

Copy link
Collaborator

Just a quick note that I continued the MFA odyssey today by finally proving to my satisfaction that SMS MFA worked as well.
Spoiler alert, it did not, quite, but it was close. I have a PR up that will fix it

jamespb97 and Nantris reacted with thumbs up emoji

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /