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(app-check): android debug token argument for app-check #6026

Conversation

glesperance
Copy link
Contributor

@glesperance glesperance commented Jan 25, 2022

Description

Debugging react-native applications with AppCheck is problematic on Android:

  1. It relies running our code on a real android device;
  2. It also required an added extra step of $adb logcat | grep DebugAppCheckProvider to extract the secret token that must then be pushed to the Firebase console. This token changes often and made the developper experience and operations much more harder.
  3. Configuring the secret token on CI relied on pushing code instead of environment variables.

This PR is a solution for that, and here is the summary of the solution:

  1. By default, the behaviour is the same as what we had before -- that is default to use the DebugAppCheckProviderFactory.
  2. If a FIREBASE_APP_CHECK_DEBUG_TOKEN env var is provided and we're in debug mode -- use that instead of the default.
  3. If not in debug mode, use the SafetyNetAppCheckProviderFactory -- like before.

IMPORTANT
Please note that I removed a test that was no longer passing because app-check doesn't fail app-attestation on android emulators with the new method anymore. See https://github.com/invertase/react-native-firebase/pull/6026/files#diff-406415e4817a8b2d1b8cd632fffc7af195be1fe7ec0a560fd0a47df5d7b03ddaL74-L91

Related issues

  1. How to activate App Check in android @react-native-firebase/app-check #5660

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

gabriel@mt react-native-firebase % yarn tests:android:test
yarn run v1.22.15
$ cd tests && yarn detox test --configuration android.emu.debug
$ /Users/gabriel/Workspace/react-native-firebase/tests/node_modules/.bin/detox test --configuration android.emu.debug
detox[25576] INFO:  [test.js] mocha --config e2e/.mocharc.js --configuration android.emu.debug --grep :ios: --invert --use-custom-logger true e2e


detox[25577] INFO:  at node_modules/jet/lib/node/vm.js:78:11
 [✈️] debugger has connected! Downloading app JS bundle...
detox[25577] INFO:  at node_modules/jet/lib/node/console.js:16:15
 Mapping auth host "localhost" to "10.0.2.2" for android emulators. Use real IP on real devices. You can bypass this behaviour with "android_bypass_emulator_url_remap" flag.
detox[25577] INFO:  at node_modules/jet/lib/node/console.js:16:15
 Mapping firestore host to "10.0.2.2" for android emulators. Use real IP on real devices. You can bypass this behaviour with "android_bypass_emulator_url_remap" flag.
detox[25577] INFO:  at node_modules/jet/lib/node/console.js:16:15
 Mapping storage host to "10.0.2.2" for android emulators. Use real IP on real devices.
detox[25577] INFO:  at node_modules/jet/lib/node/console.js:16:15
 Mapping functions host "localhost" to "10.0.2.2" for android emulators. Use real IP on real devices. You can bypass this behaviour with "android_bypass_emulator_url_remap" flag.
  appCheck()
    config
      - should configure token auto refresh in Info.plist on ios
    setTokenAutoRefresh())
      ✔ should set token refresh
detox[25577] INFO:  at node_modules/jet/lib/node/console.js:16:15
 Running "testing" with {"rootTag":11}
    getToken())
detox[25577] INFO:  at e2e/init.js:40:15

detox[25577] WARN:  at e2e/init.js:41:15
 ⚠️ A test failed:
detox[25577] WARN:  at e2e/init.js:42:15
 ️   ->  token fetch attempt should work
detox[25577] WARN:  at e2e/init.js:49:13
 ️   ->  Retrying in 5 seconds ... (1)
      ✔ token fetch attempt should work (2142ms)
    activate())
      ✔ should activate with default provider and default token refresh

detox[25577] INFO:  at e2e/init.js:55:11
  ✨ Tests Complete ✨
detox[25577] INFO:  at e2e/init.js:77:15
 Coverage data downloaded to: ./android/app/build/output/coverage//emulator_coverage.ec

  3 passing (18s)
  1 pending

✨  Done in 19.15s.

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

🔥

@vercel
Copy link

vercel bot commented Jan 25, 2022

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/BaxUPQuBunpAjfz5Emfzt3a7odwJ
✅ Preview: https://react-native-firebase-git-fork-glesperance-fea-e561c7-invertase.vercel.app

react-native-firebase-next – ./website_modular

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

[Deployment for 877df8f canceled]

@vercel
Copy link

vercel bot commented Jan 25, 2022

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-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/DASs3cR9BCvCWxGKzDK9xToE3n5J
✅ Preview: In Progress

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2022

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – react-native-firebase-next January 25, 2022 04:01 Inactive
@glesperance glesperance changed the title Feature/app check/android debug token argument feature(app-check): android debug token argument for app-check Jan 25, 2022
tests/e2e/.mocharc.js Outdated Show resolved Hide resolved
@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Waiting for User Response Blocked waiting for user response. labels Jan 25, 2022
@mikehardy
Copy link
Collaborator

One other thought - could you mention this in the app check docs for android as well? Otherwise I fear no one will discover this (not that it was easy to discover how to set it up prior either, but now it's much more usable and people should know how to access this...)

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #6026 (2c243b3) into main (49d5bed) will decrease coverage by 18.83%.
The diff coverage is 0.00%.

❗ Current head 2c243b3 differs from pull request most recent head 877df8f. Consider uploading reports for the commit 877df8f to get more accurate results

@@              Coverage Diff              @@
##               main    #6026       +/-   ##
=============================================
- Coverage     72.16%   53.34%   -18.82%     
- Complexity        0      632      +632     
=============================================
  Files           109      208       +99     
  Lines          4600    10195     +5595     
  Branches       1033     1619      +586     
=============================================
+ Hits           3319     5437     +2118     
- Misses         1203     4484     +3281     
- Partials         78      274      +196     

@glesperance glesperance changed the title feature(app-check): android debug token argument for app-check feat(app-check): android debug token argument for app-check Jan 25, 2022
@mikehardy
Copy link
Collaborator

oh - and looks like CLA is still not signed? The details link contains info on how to get it done if you could check it?

@glesperance
Copy link
Contributor Author

Updated the doc. The CLA assistant seems broken:
Screen Shot 2022-01-25 at 4 23 59 PM

@glesperance
Copy link
Contributor Author

Actually, nevermind, the CLA assistant worked, but it took a good minute or two to load.

@mikehardy
Copy link
Collaborator

Actually, nevermind, the CLA assistant worked, but it took a good minute or two to load.

Yeah - I tried it myself prompted by your mention it was busted, looks like this: cla-assistant/cla-assistant#829 - it's always something 🤷 😆

@glesperance
Copy link
Contributor Author

Ah nw nw ; I gotcha. To the risk of jinxing us, it should be good now

@mikehardy
Copy link
Collaborator

No good deed unpunished 😆

docs/app-check/usage/index.md
    109:22-109:35  warning  `pre-generated` is misspelt                                                                                                                                                                                                                                                             retext-spell  retext-spell
    109:72-109:80  warning  `buidling` is misspelt; did you mean `bridling`, `building`, `bundling`?                                                                                                                                                                                                                retext-spell  retext-spell
  111:113-111:115  warning  `eg` is misspelt; did you mean `egg`, `eh`, `pg`, `ego`, `g`, `E`, `Ea`, `Ed`, `EEG`, `Em`, `En`, `Er`, `Es`, `Eu`, `Ex`, `beg`, `cg`, `deg`, `erg`, `jg`, `keg`, `kg`, `leg`, `lg`, `meg`, `mg`, `neg`, `peg`, `reg`, `veg`, `Ag`, `EC`, `ECG`, `EKG`, `Eng`, `ET`, `Ge`, `Hg`, `VG`?  retext-spell  retext-spell
  114:179-114:181  warning  `eg` is misspelt; did you mean `egg`, `eh`, `pg`, `ego`, `g`, `E`, `Ea`, `Ed`, `EEG`, `Em`, `En`, `Er`, `Es`, `Eu`, `Ex`, `beg`, `cg`, `deg`, `erg`, `jg`, `keg`, `kg`, `leg`, `lg`, `meg`, `mg`, `neg`, `peg`, `reg`, `veg`, `Ag`, `EC`, `ECG`, `EKG`, `Eng`, `ET`, `Ge`, `Hg`, `VG`?  retext-spell  retext-spell

One of them looks like a legitimate typo (which is why we keep this check enabled...), but pre-generated and eg could maybe go in spellcheck.dict

@glesperance
Copy link
Contributor Author

glesperance commented Jan 25, 2022

Typos should be good now -- as far I understand.

$ yarn lint:spellcheck isn't working either though.

@mikehardy
Copy link
Collaborator

one last one!

  109:22-109:34  warning  `pregenerated` is misspelt; did you mean `regenerated`?  retext-spell  retext-spell

@glesperance
Copy link
Contributor Author

yep defaulted to generated...

Did I miss anything about running all those checks by myself in the contribution guidelines?

It'd be great if the checks could be run locally, and would work with a single yarn command -- otherwise we're left at going back and forth. Not sure that's the best DX.

@mikehardy
Copy link
Collaborator

It's a terrible DX and not well documented, thanks for bearing with me and working through it. I'll peel that to an issue and get it sorted, maybe a pre-push hook, or at minimum yes a single package.json run script

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.

Just the final 2 CI checks to finish now (the e2e runs for ios and android)

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Pending Merge Waiting on CI or similar labels Jan 25, 2022
@mikehardy mikehardy merged commit 6f67503 into invertase:main Jan 25, 2022
@glesperance
Copy link
Contributor Author

we did it ! Thanks for the support @mikehardy !

@glesperance
Copy link
Contributor Author

Actually question: what's the next step ; is this directly tied to a new npm release?

@mikehardy
Copy link
Collaborator

Yep was just about to kick off the release but there was a suspicious CI failure on Android e2e post merge on main branch. Once that's settled I'll release. In the meantime check our actions tab for the patches action, you'll find a patch-package zip attached on every one, so your PR can be integrated in your project even before we release, despite this being a monorepo and hard to patch

@glesperance
Copy link
Contributor Author

sounds good

@mikehardy
Copy link
Collaborator

publishing now 🚀

@glesperance
Copy link
Contributor Author

awesome!!! thanks @mikehardy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants