feat: `autoOTPVerify` for Android devices

efstathiosntonas commented 1 week ago

Introducing autoOTPVerify setting for Android devices, when this is set to false the OTP handling must be taken care from the devs instead of letting Android automagically digest the code.

This can come in handy in some scenarios where firebase auth throws The SMS code has expired when using phone number in certain Android devices.

usage, put this at the very top eg. index.js or App.tsx after firebase is initialized.

(async function () {
  try {
    await firebase.auth().settings.setAutoOTPVerify(false);
  } catch (error) {
    console.error(error);
  }
})();

manual OTP handling:

// set code and phone from textinputs
const [phone, setPhone] = useState('')
const [code, setCode] = useState('');

const handlePhoneLogin = async () => {
  const confirmResult = await auth().signInWithPhoneNumber(phone);

  await confirmResult.confirm(code).then(user => {
          console.log(user); 
  });
}

@mikehardy hi, I've been using this for like 2 years with no issues. Let me know if everything looks good (don't think I've missed something) ~and I'll update the docs too~. Can we create e2e test case for this? 🤔

Release Summary

Checklist

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

Test Plan

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

👉 Star this repo on GitHub ⭐️
👉 Follow React Native Firebase and Invertase on Twitter

vercel[bot] commented 1 week ago

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	Nov 19, 2024 10:24am

CLAassistant commented 1 week ago

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Efstathios Ntonas seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

efstathiosntonas commented 1 week ago

@mikehardy I have signed the CLA in the past 😢

efstathiosntonas commented 6 days ago

Right now it's hardcoded, I have created a patch that sets the xxL and 60 to 0L/0. I have tested the timeout (set to 0) but still I had the issue back in the days.

It's a 2 year old patch, probably this is fixed upstream but I do not dare to remove it haha, it serves me well and got 0 complaints to the day.

I do not do anything fancy on my end with Phone auth, guess it's classic Android stuff braking things around on the 30K devices out there. It was not a major issue, I had about 10-15 users complaining out of 25K back then.

ihmo it's nice to have this option even though it's anorthodox and doesn't align with Firebase SDKs. Thing is, changing these values is not even documented (last time I checked was 2 years ago).

We can extend this PR to reach modular apis too. Don't know about timeouts, before messing with xxL and 60 I've played with it but didn't work as expected.

Conclusion: If you think we should ditch this PR, let's ditch it. Maybe a poor soul will reach here from google land and apply the patch directly on ReactNativeFirebaseAuthModule.java 🤷‍♂️

ps. don't forget that we have bumped react-native like 30 times since then, maybe there was something that was messing with the listeners/activity

mikehardy commented 6 days ago

I was looking through this stuff and found a different issue you commented on:

https://github.com/firebase/quickstart-android/issues/296#issuecomment-1736910311

Looks like there is the "instant verify" case, where Play Services does some magic related to your google account on device. And there is the "SMS auto-retrieval" case, where Play Services / firebase gets the OTP before the user sees it and auto-logs in.

In your comment, you indicate that this patch (or the hard-coded version you've been using where just set it to 0L/0) disables the feature, but it was on the "instant verify" issue upstream.

Does that hard-coded patch actually stop both forms of auto-verification? If so that's unexpected, but interesting and this becomes much more interesting in my opinion

I was looking for another way to disable it by using tools:remove in AndroidManifest to disable the SMS Retriever API permission / intent but I couldn't find that so I think Firebase must be doing it some other way.

I was originally against this as an idea because I think it's all working now, but after reading the instant-verification can't-disable-it thread I think it's bigger than just auto-SMS-verify, and this might be useful - but only if your patch actually disables both in my opinion

efstathiosntonas commented 6 days ago

It blocks everything, if the user doesn’t enter the code manually it’s not digested by firebase auth at all.

Capacitor is passing it as an option:

https://github.com/capawesome-team/capacitor-firebase/blob/610a4a16fa884ca504e5f7fc53d1f91965b798be/packages/authentication/android/src/main/java/io/capawesome/capacitorjs/plugins/firebase/authentication/handlers/PhoneAuthProviderHandler.java#L61

flutterfire pass it as an option too:

https://github.com/firebase/flutterfire/blob/1ef2044a7a9f2004f933147a8494fb82fa4c3c26/packages/firebase_auth/firebase_auth/android/src/main/java/io/flutter/plugins/firebase/auth/PhoneNumberVerificationStreamHandler.java#L177

soooo, pass it as an option or as an on/off switch?

efstathiosntonas commented 6 days ago

At some point in time I believed that firebase was automagically trying to verify the phone number even if the OTP haven’t arrived yet because of the google account on the device had the (verified) phone number I was using to sign in 🤦‍♂️🤦‍♂️

mikehardy commented 6 days ago

Very interesting - all of that. Hmm. Very useful feature if it is the only way to completely disable both auto-verify capabilities. You can't reasonably test phone OTP behavior otherwise as google will likely auto-vierfiy.

We have it as an optional boolean already on one method - how do you feel about following this pattern to do all of them, vs maintaining new state via settings?

https://github.com/invertase/react-native-firebase/pull/8145/files#diff-17651da2be070111f5b3f961a4e6fda957ed253e301e14f8a1da4a5ce95c44efR294-R305

Not sure how to squeeze that into the modular API but anything that did so reasonably seems like it would work

invertase / react-native-firebase