fix(app-check): getToken returns {token: string} not string matching firebase-js-sdk

[quentin-fox]

Description

The return type of the getToken method is not of type { token: string }, but rather of type string.

On the Android side, the task result here is the AppCheckTokenResult class.

https://github.com/invertase/react-native-firebase/blob/master/packages/app-check/android/src/main/java/io/invertase/firebase/appcheck/ReactNativeFirebaseAppCheckModule.java#L85

The AppCheckTokenResult class itself has a getToken method, which returns a String.

https://github.com/firebase/firebase-android-sdk/blob/6da8617afe903126afac46ef71b1d857dcfe7ef9/appcheck/firebase-appcheck/src/main/java/com/google/firebase/appcheck/internal/DefaultAppCheckTokenResult.java#L55

Since the string is what is resolved in the Promise, the return type of the javascript method bound to this native code should also be a string.

On the iOS side, the promise is also passed the token.token, which should be a string, instead of the token.

https://github.com/invertase/react-native-firebase/blob/master/packages/app-check/ios/RNFBAppCheck/RNFBAppCheckModule.m#L101

In our own app, we found that at runtime the getToken call was returning a string, so we had to use typescript assertions to prevent compiler errors due to the incorrect type.

Related issues

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
  • Somewhat? (it may break existing typescript builds, but they would have been typed incorrectly anyways)

Test Plan

I added a few tests to assert that getToken returns a string. It should have already be returning this type, as in the tests it is passed into jwt.decode, which accepts a string.

---

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]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/CJrkUhv5eJQ136v66EZ1JJ3A4SPF
✅ Preview: https://react-native-firebase-git-fork-quentin-fox-main-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/7t89qCGUzn1AcpGsqb1YcAxocJpb
✅ Preview: Canceled

[Deployment for 31c4498 canceled]

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #5979 (1def9c3) into main (8efda50) will decrease coverage by 0.09%.
The diff coverage is 5.56%.

❗ Current head 1def9c3 differs from pull request most recent head 31c4498. Consider uploading reports for the commit 31c4498 to get more accurate results

@@             Coverage Diff              @@
##               main    #5979      +/-   ##
============================================
- Coverage     53.04%   52.95%   -0.08%     
  Complexity      622      622              
============================================
  Files           208      208              
  Lines         10171    10189      +18     
  Branches       1618     1619       +1     
============================================
+ Hits           5394     5395       +1     
- Misses         4523     4540      +17     
  Partials        254      254              

[mikehardy]

Mmm - first of all, thanks for noticing this, second 🙋 I made this mistake, sorry about that, but third - I think we need to go other direction and return (or somehow simulate and return) an AppCheckTokenResult for real. https://firebase.google.com/docs/reference/js/app-check.md#gettoken - we're trying to mirror the firebase-js-sdk where ever possible and though the AppCheck APIs diverge a bit I think we can get there with this one. Do you think it's possible to actually return AppCheckTokenResult, and alter the return values to be like that? It won't be a breaking change since AppCheck is in beta but I will make certain the release notes warn people in big bold terms so they're paying attention, if it's possible

[quentin-fox]

It should be simple enough to do! I'll quickly rejig the PR so that it confirms to the firebase-js-sdk.

[quentin-fox]

I couldn't for the life of me get the android project in packages/app-check/android to Gradle sync in my version of android studio, so there might be errors. Not at all experienced writing code on the bridge, so please tear it apart if it's not done correctly.

[mikehardy]

Ah. sorry - the general development style is to actually load the tests/android directory into gradle. It has all of the modules hooked into it correctly for live editing while using the tests app as the "module host", and allowing quick re-runs in an e2e test harness context --> https://github.com/invertase/react-native-firebase/blob/main/tests/README.md

Regardless, I'll look through it - thanks!

[mikehardy]

This looks correct - originally on review I was thinking we needed to have expireTimeMillis in there as well but when I examined the firebase-js-sdk reference, it really is just an object with nothing but a token: string property in it, so this seems exactly right. I'll approve the CI runs and we'll see how it goes :-)