Skip to content
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

feat: autoOTPVerify for Android devices #8145

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

Conversation

efstathiosntonas
Copy link
Contributor

@efstathiosntonas efstathiosntonas commented Nov 19, 2024

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.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • 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 Nov 19, 2024

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
Copy link

CLA assistant check
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 efstathiosntonas changed the title feat: introduced autoOTPVerify for Android devices feat: autoOTPVerify for Android devices Nov 19, 2024
@efstathiosntonas
Copy link
Contributor Author

@mikehardy I have signed the CLA in the past 😢

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.

Interesting - looking at this to see how it fits with the API...firebase-js-sdk has nothing to say on the subject because it can't auto-verify so we have no guidance really

But the same can be said for the existing "timeout" you can provide on verifyPhoneNumber - https://rnfirebase.io/reference/auth#verifyPhoneNumber

(though for the multifactor and reauthenciate cases currently, there appears to be no timeout setting available - only in verify it seems)

Question - Have you attempted to get the root cause of the auto-verifier consuming these codes but then not actually verifying on your behalf? That seems like a fundamental / vital error and my read on the SMS Retriever API makes that seem unlikely to happen without correctly broadcasting to the app which would then call the listener...and we don't get many complaints from this from people that have implemented the listener so I'm curious what the frequency of this is

Question - Is this something that you enable and disable dynamically? That is does it need to be a programmable / mutable property at runtime? Or is this something that you disable completely? (it could potentially be achieved in firebase.json or via AndroidManifest.xml manipulation if it was just a full disable)

Implementation thoughts

  • what about the modular APIs? The non-modular ones are in the middle of a deprecation process now and will go away, we can only accept API modifications if they modify all the related modular stuff index.d.ts and index.js as well
    what about reauthenticateWithPhoneNumber?

  • what if all the APIs allowed a timeout specification similar to verifyPhoneNumber? I don't particularly like that since it deviates even further from firebase-js-sdk - a settings / config change seems better, just curious if you've contemplated it

@efstathiosntonas
Copy link
Contributor Author

efstathiosntonas commented Nov 19, 2024

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
Copy link
Collaborator

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

firebase/quickstart-android#296 (comment)

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
Copy link
Contributor Author

efstathiosntonas commented Nov 19, 2024

@efstathiosntonas
Copy link
Contributor Author

efstathiosntonas commented Nov 19, 2024

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
Copy link
Collaborator

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

Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Dec 17, 2024
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