[Wallet] Add reCaptcha verification behind feature flag #5630

gnardini · 2020-10-29T21:01:50Z

Description

Adds reCaptcha implementation behind a feature flag. If KOMENCI is true, after clicking on Start on the VerificationEducationScreen the user will be prompted with a reCaptcha challenge that they'll need to complete before being able to verify. It also adds SafetyNet which is a check for Android devices.

This is missing integration with KomenciKit

Tested

By changing the value of the KOMENCI flag and seeing the captcha flow (captcha code is logged)

Related issues

Part of Integration support for feeless attestation #5426

gnardini · 2020-10-29T21:15:08Z

packages/mobile/android/app/src/main/res/values/strings.xml

@@ -0,0 +1,3 @@
+<?xml version="1.0" encoding="utf-8"?>


I know this file looks weird, it's because of SafetyNet. It depends on react-native-securerandom which doesn't support automatic linking, and when doing it manually it threw an error because this file was missing.

This manual linking is the reason for the changes to the settings.gradle and the Podfile as well.

Would love to fork/fix/upstream after wednesday as good citizens of the open source world

Yeah, I like doing that sort of stuff too, but the maintainers don't seem very active. The first reason I had to fork was to add support for AndroidX since it was failing for that reason. There's a PR that adds that from almost a year ago: rajivshah3/react-native-google-safetynet#283 which has this message:

Thanks for the PR! I can review and investigate the build failure in a few days when I have some time

(December 2019)

gnardini · 2020-10-29T21:17:05Z

packages/mobile/src/transactions/__snapshots__/TransferConfirmationCard.test.tsx.snap

@@ -108,17 +108,26 @@ exports[`TransferConfirmationCard renders correctly for CELO withdrawal transact
                  </svg>
                </View>
              </View>
-              <Text
+              <View


This changes are due to the change to the implementation of Expandable. It changes nothing visually, it's just wrapping the Text with a View.

cmcewen · 2020-10-29T22:54:38Z

packages/mobile/locales/en-US/onboarding.json

-    "body": "To make sure your number is really yours, we’re going to send you three messages that will cost about 0.05 cUSD each.\n\nConfirming your number takes about three minutes.",
+    "title": "Phone Number",
+    "header": "Connect your phone number",
+    "body": "Connecting takes about three minutes. To confirm your number, you’ll receive three messages.",


Do we need to keep this text for the fallback version? cc @tarikbellamine

Yeah good catch. We will want to keep all the old text because both feeless and non-feeless flows will be possible.

cmcewen · 2020-10-29T22:55:18Z

packages/mobile/package.json

@@ -107,6 +107,7 @@
    "react-native-fs": "^2.16.6",
    "react-native-gesture-handler": "^1.6.1",
    "react-native-geth": "https://github.com/celo-org/react-native-geth#5864c09",
+    "react-native-google-safetynet": "https://github.com/celo-org/react-native-google-safetynet#e698d70",


not sure if you created a PR to get this upstreamed - but usually nice to do

cmcewen · 2020-10-29T22:57:25Z

packages/mobile/src/verify/VerificationEducationScreen.tsx

@@ -47,6 +60,12 @@ function VerificationEducationScreen({ route, navigation }: Props) {
  const numberVerified = useSelector(numberVerifiedSelector)
  const partOfOnboarding = !route.params?.hideOnboardingStep

+  const country = useMemo(() => {
+    const regionCode = getRegionCode(e164PhoneNumber || '')
+    const countries = new Countries(i18n.language)


this looks like it could be expensive - but just a note that useMemo is not always necessary https://blog.logrocket.com/rethinking-hooks-memoization/

That was a great article, I've specially been guilty of 4. Using useMemo solely for referential equalities in the past.

I didn't benchmark how expensive the call is tbh, but it's iterating and applying transformations over a list, if there are many re-renders it could get expensive I guess. I'll make sure not to abuse the use of useMemo :)

cmcewen · 2020-10-29T22:58:39Z

packages/mobile/src/verify/VerificationEducationScreen.tsx

+  const showCaptcha = async () => {
+    setIsCaptchaVisible(true)
+    const safetyNetAttestationResponse = await getSafetyNetAttestation()
+    Logger.info('SafetyNet attestation complete:', JSON.stringify(safetyNetAttestationResponse))


is this response big? do we need to log it all?

This is an example response:

{ "apkCertificateDigestSha256": ["+sYXRdwJA3hvue3mKpYrOZ9zSPC7b4mbgzJmdZEDO5w="], "apkDigestSha256": "5UVIjg1t6wE5CfIhgRMiXQAmB7PzUpSUyGuirP/qyOs=", "apkPackageName": "org.celo.mobile.alfajores.dev", "basicIntegrity": true, "ctsProfileMatch": true, "evaluationType": "BASIC", "nonce": "IdJQ46NEBT214jlGZ036i7vZq81j/dBiUOAR6HEM", "timestampMs": 1603991278194 }

Probably not all fields are necessary, but I thought they might be useful.

cmcewen · 2020-10-29T23:00:35Z

packages/mobile/src/verify/VerificationEducationScreen.tsx

+    const captchaCode = res?.nativeEvent?.data
+    if (captchaCode !== 'cancel' && captchaCode !== 'error') {
+      Logger.info('Captcha code received: ', captchaCode)
+      // TODO: Add some brief loading indicator and send captcha somewhere before continuing.


given the feature flag this is okay, but normally i would request some changes 😛

Yeah, this PR by itself is not super useful, it's just laying the foundation for the next one when this is integrated with KomenciKit :D

cmcewen · 2020-10-29T23:04:03Z

packages/mobile/src/verify/VerificationEducationScreen.tsx

        {firstButton}
        <View style={styles.spacer} />
        <TextButton style={styles.doINeedToConfirmButton} onPress={onPressLearnMore}>
          {t('verificationEducation.doINeedToConfirm')}
        </TextButton>
      </ScrollView>
+      <Modal isVisible={isCaptchaVisible} style={styles.recaptchaModal}>


i mentioned this to cal, but we should just be mindful of modals from a perf perspective - can lead to overdraw https://developer.android.com/topic/performance/rendering/overdraw

Yes, we do use these modals in a bunch a places already, we can address this separately.

cmcewen · 2020-10-29T23:04:46Z

packages/mobile/src/verify/safety/GoogleReCaptcha.tsx

+			</head>
+			<body> 
+				<div id="captcha" style="height: 100vh;">
+          <div style="text-align: center; height: 100%; display: flex; flex-direction: column; justify-content: center; align-items: center;">


could you fix the indents here? (just kidding 😄 )

Run it through a formatter so it looks nicer :)

cmcewen · 2020-10-29T23:06:16Z

packages/mobile/src/verify/safety/GoogleReCaptcha.tsx

+			</html>`
+  return (
+    <WebView
+      originWhitelist={['*']}


does this have to be *? it looks like we maybe want https? https://github.com/react-native-webview/react-native-webview/blob/master/docs/Reference.md#originwhitelist

tarikbellamine · 2020-10-30T06:19:38Z

packages/mobile/src/verify/VerificationEducationScreen.tsx

+        <GoogleReCaptcha
+          siteKey={RECAPTCHA_SITE_KEY}
+          url={WEB_LINK}
+          languageCode="en"


Don't we want this to be whatever the app language is?

tarikbellamine

LGTM.

Please make sure you've thoroughly tested on both OS's and specifically the duplication of the phone number input part.

jeanregisser

Looking good! 👍 See small comment below.

jeanregisser · 2020-10-30T17:17:45Z

packages/mobile/src/verify/VerificationEducationScreen.tsx

        {firstButton}
        <View style={styles.spacer} />
        <TextButton style={styles.doINeedToConfirmButton} onPress={onPressLearnMore}>
          {t('verificationEducation.doINeedToConfirm')}
        </TextButton>
      </ScrollView>
+      <Modal isVisible={isCaptchaVisible} style={styles.recaptchaModal}>


Yes, we do use these modals in a bunch a places already, we can address this separately.

jeanregisser · 2020-10-30T17:32:43Z

yarn.lock

+react-native-securerandom@^0.1.1:
+  version "0.1.1"
+  resolved "https://registry.yarnpkg.com/react-native-securerandom/-/react-native-securerandom-0.1.1.tgz#f130623a412c338b0afadedbc204c5cbb8bf2070"
+  integrity sha1-8TBiOkEsM4sK+t7bwgTFy7i/IHA=
+  dependencies:
+    base64-js "*"
+
+react-native-securerandom@^1.0.0:
+  version "1.0.0"
+  resolved "https://registry.yarnpkg.com/react-native-securerandom/-/react-native-securerandom-1.0.0.tgz#1cff2f727c90c9ec3318b42dbf825a628b53b49b"
+  integrity sha512-lnhcsWloFzMN/HffyDBlh4VlqdhDH8uxEzUIH3aJPgC1PxV6OKZkvAk409EwsAhcmG/z3yZuVKegKpUr5IM9ug==
+  dependencies:
+    base64-js "*"
+


We shouldn't add 2 versions of the same depdency here.

Gonzalo Nardini added 2 commits October 29, 2020 17:49

[Wallet] Add reCaptcha verification behind feature flag

1d75462

Update snapshots

91ca156

gnardini requested review from aaronmgdr, asaj, cmcewen, i1skn, jeanregisser, jmrossy and timmoreton as code owners October 29, 2020 21:01

gnardini added the wallet label Oct 29, 2020

Updte branding sha

48c9055

gnardini commented Oct 29, 2020

View reviewed changes

Gonzalo Nardini added 3 commits October 29, 2020 18:21

Fix linter

68f6174

Update snapshot

97828e3

Fix podfile

0617ae2

cmcewen reviewed Oct 29, 2020

View reviewed changes

Soem PR fixes

56f4620

tarikbellamine reviewed Oct 30, 2020

View reviewed changes

tarikbellamine approved these changes Oct 30, 2020

View reviewed changes

Use app language

5e4ce1b

jeanregisser approved these changes Oct 30, 2020

View reviewed changes

aaronmgdr removed their request for review October 30, 2020 17:46

Gonzalo Nardini added 2 commits October 30, 2020 15:13

Remvoe unused version of library

8ec87ba

update snapshot

5bebc12

gnardini added the automerge Have PR merge automatically when checks pass label Oct 30, 2020

mergify bot and others added 4 commits October 30, 2020 19:36

Merge branch 'master' into captcha

2dab9f8

update snapshot

e514082

Merge branch 'master' into captcha

5b7447e

Try removing --runInBand

0981e14

mergify bot and others added 4 commits October 30, 2020 22:57

Merge branch 'master' into captcha

55e235f

Add max-workers=2

d913151

Merge branch 'master' into captcha

3fd8011

Merge branch 'master' into captcha

779914d

gnardini removed the automerge Have PR merge automatically when checks pass label Oct 31, 2020

Fix config import

c815d08

gnardini added the automerge Have PR merge automatically when checks pass label Oct 31, 2020

mergify bot merged commit c6ebf06 into master Oct 31, 2020

mergify bot deleted the captcha branch October 31, 2020 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Add reCaptcha verification behind feature flag #5630

[Wallet] Add reCaptcha verification behind feature flag #5630

gnardini commented Oct 29, 2020

gnardini Oct 29, 2020

cmcewen Oct 29, 2020

gnardini Oct 30, 2020

gnardini Oct 29, 2020

cmcewen Oct 29, 2020

tarikbellamine Oct 30, 2020

cmcewen Oct 29, 2020

cmcewen Oct 29, 2020

gnardini Oct 30, 2020

cmcewen Oct 29, 2020

gnardini Oct 30, 2020

cmcewen Oct 29, 2020

gnardini Oct 30, 2020

cmcewen Oct 29, 2020

jeanregisser Oct 30, 2020

cmcewen Oct 29, 2020

gnardini Oct 30, 2020

cmcewen Oct 29, 2020

tarikbellamine Oct 30, 2020

tarikbellamine left a comment

jeanregisser left a comment

jeanregisser Oct 30, 2020

jeanregisser Oct 30, 2020 •

edited

Loading

[Wallet] Add reCaptcha verification behind feature flag #5630

[Wallet] Add reCaptcha verification behind feature flag #5630

Conversation

gnardini commented Oct 29, 2020

Description

Tested

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikbellamine left a comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

jeanregisser Oct 30, 2020 •

edited

Loading