feat(auth, oauth): support native oauth providers

[lpfrenette]

Description

Fixed the code for ios for using microsoft.com oauth provider.
Used the PR 7187 from https://github.com/KielKing/react-native-firebase/tree/feature/Support_Native_OAuth_Providers

Related issues

• Closes feat(auth): Add support for native federated OAuth Providers (Microsoft, Yahoo) #7187 - based on that PR, supercedes it

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

Screenshot of successful login

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]

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 27, 2023 0:44am
react-native-firebase-next	❌ Failed (Inspect)			Nov 27, 2023 0:44am

[CLAassistant]

All committers have signed the CLA.

[mikehardy]

Interesting! I take it you are using this then (or integrating it currently, with plans to use it) or you probably would not have noticed that subtle parameters / customParameters difference I see in the last commit?

I would love to know what your current usage status is (integrating, already in testing, out in production) - knowing it is in production gives me a lot of confidence in merging a PR besides just me looking at it, as I mentioned in the other PR I think

This all looks reasonable, and the original PR also looked reasonable - I'd be happy to see this PR go in and enable the support.

[lpfrenette]

I'm using it in a project not yet in production. It's for a customer that has everything on microsoft so it just make sense to use microsoft oauth . And with firebase authentication, it makes it very easy to use.

[Milutin-P]

Hey everyone, I'm not sure if it's okay for me to jump in like this.

I'm currently in the midst of integrating Microsoft's OAuth functionalities into my project and came across your pull request, which precisely addresses my needs. I've succeeded with OIDC method.

I wanted to ask if there is a timeline on when will this be available? Also, if there’s anything specific I can do to help expedite this process.

Thank you for your hard work on this project.
🔥

[mikehardy]

Hey @Milutin-P 👋 no problem jumping in like this and I appreciate the prompt - this one is marked pending merge which is a note to myself that it appears to be ready to go. And the "waiting for user response" was just to do a quick survey on usage to make sure someone was using it, I got a positive response to that.

Let me do a quick scan to make sure I'm not missing anything, then merge this + release

[lpfrenette]

I've been using it, for two weeks now. The project is currently in internal testing phase and have no issue to report.

[Milutin-P]

Thank you so much for the quick response. It's great to hear that the pull request is ready and that my input contributed to confirming its usage within the community. I'm looking forward to the release 🚀

Thank you both for your dedication and support. I'll let you know if I have any issues with it.

[mikehardy]

@lpfrenette are you okay if I push changes to this PR on your branch in your repo? Not sure how that will affect your work flow - I'm ready to merge the functionality but I need to change the API shape so it conforms to the firebase-js-sdk docs, as we have a stated goal of being a drop-in replacement for firebase-js-sdk with respect to APIs

In particular, in place of signInWithProvider(provider) the implementation needs to be underneath the signInWithRedirect and signInWithPopup APIs (per https://firebase.google.com/docs/auth/web/microsoft-oauth#handle_the_sign-in_flow_with_the_firebase_sdk)

Similarly, in place of linkWithProvider it needs to be an implementation of linkWithCredential to match https://firebase.google.com/docs/auth/web/microsoft-oauth#expandable-1

I'm happy to make this change and I have the stated goal while making the change that the functionality continues exactly the same, just under different API.

I also need to export the functionality with compatible API shape for the new firebase-js-sdk v9 modular style, which shouldn't affect functionality but does require a code addition

If you don't want me pushing to your branch I can make a new PR with the changes described above on a branch I create in the main repo, no issue either way

[lpfrenette]

Sure you can push in my branch, i've added you as a collaborator. :)

[mikehardy]

@lpfrenette most people don't know this but the default setting for fork/branches in a fork-based repo is that the upstream repository maintainer can push direct to your fork/branch that backs the PR just 👊 -

Maintainers are allowed to edit this pull request.

...so I already had the ability and I don't need to be a formal collaborator but it seems rude as a maintainer to just do that without asking 😆 . Either way, I'll hop on it and see if I can't get this thing landed

Thanks!

[mikehardy]

rebased against main, and APIs (with associated docs) altered in javascript such that these APIs exist now:

auth.signInWithPopup + auth.signInWithRedirect - both use same / original native methods from before
user.linkWithPopup + user.linkWithRedirect - in place of previous linkWithProvider, but work exactly the same, same native code
user.reauthenticateWithPopup + user.reauthenticateWithRedirect - works the way you expect

they all take "provider" as a param, created as before

...those methods are there for v8 and v9 styles and I think I got the docs + typescript types and everything correct - I'll hear about it from you or a future user if not certainly :-)

pushed that all to the fork/branch backing this. iOS will fail in CI for unrelated reasons, the e2e suite passes locally.

I'll let it sit like this probably until Sunday evening / Monday morning in case you or anyone has a chance to review, then I'll address comments if any + merge + release

Cheers!

[codecov]

Codecov Report

Merging #7443 (4608d3d) into main (8673f31) will decrease coverage by 35.07%.
The diff coverage is 0.00%.

❗ Current head 4608d3d differs from pull request most recent head c15f51c. Consider uploading reports for the commit c15f51c to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #7443       +/-   ##
=============================================
- Coverage     53.72%   18.64%   -35.07%     
+ Complexity      738      309      -429     
=============================================
  Files           248      248               
  Lines         12163    12324      +161     
  Branches       1897     1924       +27     
=============================================
- Hits           6533     2297     -4236     
- Misses         5279     9291     +4012     
- Partials        351      736      +385     

[lpfrenette]

Just applied the patches with the new change, and after a little code refactoring in my app everything works like a charm

[mikehardy]

The review and the success report are invaluable @Milutin-P and @lpfrenette - really appreciated.
There was a tiny (literally invisible...) spacing issue that the lint:markdown run in CI picked up, I pushed a commit for that, all the other CI checks were green (and as mentioned iOS e2e runs locally, it just has flaky-CI behavior temporarily...)

Once I get the various lint checks to go - none of which will affect behavior a bit - I'll merge and release this now

Huge credit to @KielKing for the original PR, we've all been working hard on it of course but we are for the most part carrying your original work here to completion after a few months of delay. All the effort is really appreciated!

[Milutin-P]

Thank you both immensely for your commitment and hard work.
I'm looking forward to testing it today/tomorrow.

[Milutin-P]

@mikehardy @lpfrenette Something is wrong. I've used the example from the docs:

      const provider = new auth.OAuthProvider('microsoft.com');
      provider.addScope('offline_access');
      auth().signInWithRedirect(provider);

And I get a typescript error saying Property 'addScope' does not exist on type 'AuthProvider'.ts(2339)

Integration works fine, that's good. But the interface for OAuthProvider is not good. Here's a link to the index.d.ts file.

[mikehardy]

If it is just types, that's great - those are usually easy to fix, especially given we have firebase-js-sdk as a reference. I'll take a look

[Milutin-P]

@mikehardy @lpfrenette
I devoted several hours to resolving this issue, but without success. A detailed comparison with the firebase-js-sdk/packages/auth-types index.d.ts file did not yield the expected results. It appears we might have missed this discrepancy because our tests are in JavaScript, which doesn’t flag TypeScript errors.

Additionally, I observed that in our index.d.ts, the new keyword is not employed. There's also an eslint ignore comment present:

// eslint-disable-next-line @typescript-eslint/no-misused-new
    new (providerId: string): AuthProvider;`

Naturally, since I'm very stubborn, it will overwhelm me at some point and I won't find peace until I resolve it. So, I'll try again at some point.