Authentication screen #58

jrbuhl93 · 2021-06-20T22:55:29Z

Feature for #15

I have added biometric authentication using this library: https://github.com/hieuvp/react-native-fingerprint-scanner. The library should work with all devices according to the documentation including those that use face id instead of fingerprint and ios. I have only thoroughly tested with my device: an Android Pixel 2 that uses fingerprint authentication. If the library does not detect a biometry sensor available, the app should go into the Primary screen (untested).

The authentication in this pull request is mandatory and cannot be turned off in the settings. If you think this is something users would want, Opt-in/out authentication would require a boolean property added to the user object (on servers too) and a another item in the settings.

I hope we can thoroughly test this feature before merging it into the main codebase.

app/screens/authentication-screen/biometry-check-screen.tsx

nicolasburtey · 2021-06-21T12:00:00Z

app/screens/authentication-screen/biometry-check-screen.tsx

+  })
+
+  const checkIfBiometryIsAvailable = async () => {
+    const biometryIsAvailable = await isSensorAvailable()


are there some odds that isSensorAvailable could throw?

Reworked it to be then/catch

app/screens/move-money-screen/money-money-screen.tsx

app/utils/biometricAuthentication.ts

jrbuhl93 · 2021-06-21T15:10:41Z

I should mention https://github.com/hieuvp/react-native-fingerprint-scanner is no longer being actively maintained by the owner: hieuvp/react-native-fingerprint-scanner#178. I'm wasn't able to find an actively maintained library that seemed like a good fit. https://github.com/oblador/react-native-keychain seems maintained and GaloyMoney already has it installed, but I was running into issues with it on my Android Pixel 2.

Also, how do you think the user experience of this feature will be? I could see it being a problem if some users have dirt on their hands from working, but I haven't been to El Zonte yet. An alternate way to authenticate besides logging out and then back in might be needed.

nicolasburtey · 2021-06-21T16:00:56Z

A quick test.

I have tried it on an iPhone X with FaceId (have not tried android yet) and the flow is a bit awkward: after login in, nothing really changes as of now.

but then, when you restart the app, it's asking if you want to set up FaceId at the second launch. this is not the place to ask for FaceId setup. this should be either after a succesful login, or in the settings tab.

I have not tried the flow of clicking on "don't allow" here. something to give a look at.

also, if you log out, the faceId registration is not being reset. need to re-install the app for that.

But otherwise the faceId works as expected after it has been set up. if you don't faceId you can't log in. I think there could be a fallback with a pin number of the iOS. that would be nice to have.

cryptosalomao · 2021-06-24T15:09:43Z

app/screens/settings-screen/settings-screen.tsx

+  const securityAction = () => {
+    var isBiometricsEnabled = null
+    var isPinEnabled = null
+
+    RNSecureKeyStore.get("isBiometricsEnabled")
+      .then((res) => {
+        if (isPinEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: true, mIsPinEnabled: isPinEnabled })
+        } else {
+          isBiometricsEnabled = true
+        }
+      }, (err) => {
+        if (isPinEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: false, mIsPinEnabled: isPinEnabled })
+        } else {
+          isBiometricsEnabled = false
+        }
+      })
+
+    RNSecureKeyStore.get("PIN")
+      .then((res) => {
+        if (isBiometricsEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: isBiometricsEnabled, mIsPinEnabled: true })
+        } else {
+          isPinEnabled = true
+        }
+      }, (err) => {
+        if (isBiometricsEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: isBiometricsEnabled, mIsPinEnabled: false })
+        } else {
+          isPinEnabled = false
+        }
+      })
+
+  }


Let's make this an util function in case we need this check somewhere else in the future

This sounds like pre-mature optimisation.

in case we need

I think its fine to keep it in the this file as long as its the only place we're using it.

Not a big deal though.

bodymindarts

Looks good over all.... see comments for small cleanup suggestions.

app/screens/authentication-screen/pin-screen.tsx

app/screens/settings-screen/security-screen.tsx

bodymindarts · 2021-06-24T17:14:23Z

app/screens/settings-screen/security-screen.tsx

+  const [isPinEnabled, setIsPinEnabled] = useState(mIsPinEnabled)
+
+  useFocusEffect(() => {
+    RNSecureKeyStore.get("isBiometricsEnabled")


Can these strings "PIN" and "isBiometricsEnabled" be extracted into const variables?

app/screens/settings-screen/security-screen.tsx

app/screens/settings-screen/settings-screen.tsx

bodymindarts · 2021-06-24T17:19:13Z

app/screens/settings-screen/settings-screen.tsx

+  const securityAction = () => {
+    var isBiometricsEnabled = null
+    var isPinEnabled = null
+
+    RNSecureKeyStore.get("isBiometricsEnabled")
+      .then((res) => {
+        if (isPinEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: true, mIsPinEnabled: isPinEnabled })
+        } else {
+          isBiometricsEnabled = true
+        }
+      }, (err) => {
+        if (isPinEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: false, mIsPinEnabled: isPinEnabled })
+        } else {
+          isBiometricsEnabled = false
+        }
+      })
+
+    RNSecureKeyStore.get("PIN")
+      .then((res) => {
+        if (isBiometricsEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: isBiometricsEnabled, mIsPinEnabled: true })
+        } else {
+          isPinEnabled = true
+        }
+      }, (err) => {
+        if (isBiometricsEnabled !== null) {
+          navigation.navigate("security", { mIsBiometricsEnabled: isBiometricsEnabled, mIsPinEnabled: false })
+        } else {
+          isPinEnabled = false
+        }
+      })
+
+  }


This sounds like pre-mature optimisation.

in case we need

I think its fine to keep it in the this file as long as its the only place we're using it.

Not a big deal though.

app/utils/biometricAuthentication.ts

app/utils/logout.ts

bodymindarts · 2021-06-24T17:30:25Z

Actually had a thought regarding the accesses with magic strings ie: RNSecureKeyStore.get("PIN"). Perhaps you could wrap access to RNSecureKeyStore. In a dedicated object and use it via:

KeyStoreWrapper.getPin()

I think that would make it cleaner all together. Group all functions regarding that KeyStore in a wrapper.

nicolasburtey · 2021-07-02T13:55:10Z

app/screens/authentication-screen/authentication-check-screen.tsx

+export const AuthenticationCheckScreen: ScreenType = ({ navigation }: Props) => {
+  useFocusEffect(() => {
+    checkForAuthentication()
+  })


useEffect(() => { checkForAuthentication() }, [])

nicolasburtey · 2021-07-02T13:59:39Z

app/screens/authentication-screen/authentication-check-screen.tsx

+    checkForAuthentication()
+  })
+
+  const checkForAuthentication = async () => {


put function within useEffect scope

nicolasburtey · 2021-07-02T14:03:04Z

app/screens/authentication-screen/authentication-screen.tsx

+  navigation: any,
+}
+
+export const AuthenticationScreen: ScreenType = ({ route, navigation }: Props) => {


add type related to route.params

nicolasburtey · 2021-07-02T14:04:36Z

app/screens/authentication-screen/authentication-screen.tsx

+    attemptAuthentication()
+  })
+
+  const attemptAuthentication = () => {


I don't think attemptAuthentication needs to be async even though BiometricWrapper.authenticate is async. Unless we want to await a return value of BiometricWrapper.authenticate instead of using callbacks. Is there another reason to declare attemptAuthentication async?

nicolasburtey · 2021-07-02T14:05:23Z

app/screens/authentication-screen/authentication-screen.tsx

+    } else {
+      description = translate("AuthenticationScreen.setUpAuthenticationDescription")
+    }
+    BiometricWrapper.authenticate(description, handleAuthenticationSuccess, handleAuthenticationFailure)


add 1 line comment on what this does

nicolasburtey · 2021-07-02T14:10:59Z

app/screens/authentication-screen/pin-screen.tsx

+        ]
+      })
+    } else {
+      if (pinAttempts < 2) {


should be const outside of the loop

nicolasburtey · 2021-07-02T14:24:54Z

app/screens/settings-screen/settings-screen.tsx

+    category: translate('common.security'),
+    icon: 'lock-closed-outline',
+    id: 'security',
+    action: () => securityAction(),


action: securityAction

nicolasburtey · 2021-07-07T15:22:11Z

some text cropped on iOS:

nicolasburtey · 2021-07-07T16:18:37Z

App running in the background. When coming back to the app, not asking for faceId again. But I think it ask back FaceId because the app can stay in the background for a long time.

nicolasburtey · 2021-07-07T16:20:35Z

If I go to setPin, I am being stuck on this screen. I can put 4 digits and nothing happens.

nicolasburtey · 2021-07-07T16:25:27Z

after too many attempt of bad pin, the "too many attempt to login, log out" text appeared maybe only for 0.2 seconds. I could barely see it before the screen had reset.

jrbuhl93 · 2021-07-07T17:09:23Z

Would you like me to add authentication when the device goes from background -> active. I have not implemented this yet, only when the app starts from terminated. Also do you think the best way to handle the "too many attempt to login, log out" is to add an await sleep(1000)?

nicolasburtey · 2021-07-07T17:22:54Z

Would you like me to add authentication when the device goes from background -> active. I have not implemented this yet, only when the app starts from terminated

yes. this could be done as a follow up PR.

nicolasburtey · 2021-07-07T17:23:50Z

Also do you think the best way to handle the "too many attempt to login, log out" is to add an await sleep(1000)?

can be a good option yes. or it could be a popup. and when you click on ok you're being redirected at this stage.

nicolasburtey · 2021-07-07T17:24:32Z

If I go to setPin, I am being stuck on this screen. I can put 4 digits and nothing happens.

I have this issue on Android as well. this is the biggest issue I see so far on my end.

…cropping

nicolasburtey reviewed Jun 21, 2021

View reviewed changes

app/screens/authentication-screen/biometry-check-screen.tsx Outdated Show resolved Hide resolved

nicolasburtey reviewed Jun 21, 2021

View reviewed changes

app/screens/move-money-screen/money-money-screen.tsx Outdated Show resolved Hide resolved

nicolasburtey reviewed Jun 21, 2021

View reviewed changes

app/utils/biometricAuthentication.ts Outdated Show resolved Hide resolved

cryptosalomao reviewed Jun 24, 2021

View reviewed changes

bodymindarts reviewed Jun 24, 2021

View reviewed changes

bodymindarts self-requested a review June 27, 2021 18:46

jrbuhl93 force-pushed the authentication-screen branch from 7f7fd29 to 2e22710 Compare June 29, 2021 22:17

nicolasburtey reviewed Jul 2, 2021

View reviewed changes

bodymindarts assigned jrbuhl93 Jul 2, 2021

jrbuhl93 requested a review from nicolasburtey July 2, 2021 20:46

bodymindarts assigned nicolasburtey Jul 6, 2021

jrbuhl93 added 5 commits July 6, 2021 11:58

install react-native-fingerprint-scanner

f2d39e1

package.json

17293ca

biometric authentication

7205726

use replace navigation from biometry check

f3ac2a1

fix FaceID message

8ddce7b

jrbuhl93 added 14 commits July 6, 2021 12:00

review fixes

7c27d2d

security settings and backup pin code

ec393a0

status bar on move money screen is in jsx instead of useEffect

dc1865a

android linking

a6690ad

ios linking and setPinFailedMatch text

d40581c

EOF

98e00db

remove screen import comments

fcc5541

remove commented out icon

715f88b

reduce repitition in pin pad jsx

8af4ed9

Wrap SecureKeyStore and Biometric utility functions

eb833fd

Fix comments and some linting

b8704e5

Add typing to route params, remove magic numbers, misc fixes

3589501

Refactor resetPinAttempts

501a6cd

Rebase app and fix lint warnings

ff913f0

jrbuhl93 force-pushed the authentication-screen branch from 79af9a7 to ff913f0 Compare July 6, 2021 18:12

Fix pods after rebase

cebed74

jrbuhl93 added 2 commits July 7, 2021 12:25

Fix setPIN button and add 1 second wait after failed pin

73e5c78

Increase marginRight on text in security screen to prevent iphone 11 …

c6d2007

…cropping

nicolasburtey merged commit 44beaef into GaloyMoney:main Jul 7, 2021

jrbuhl93 deleted the authentication-screen branch July 8, 2021 17:08

jrbuhl93 mentioned this pull request Jul 15, 2021

use biometrics if available when opening the application #15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication screen #58

Authentication screen #58

jrbuhl93 commented Jun 20, 2021 •

edited

Loading

nicolasburtey Jun 21, 2021

jrbuhl93 Jun 21, 2021

jrbuhl93 commented Jun 21, 2021 •

edited

Loading

nicolasburtey commented Jun 21, 2021

cryptosalomao Jun 24, 2021

bodymindarts Jun 24, 2021

bodymindarts left a comment

bodymindarts Jun 24, 2021

bodymindarts Jun 24, 2021

bodymindarts commented Jun 24, 2021

nicolasburtey Jul 2, 2021

nicolasburtey Jul 2, 2021

nicolasburtey Jul 2, 2021

nicolasburtey Jul 2, 2021

jrbuhl93 Jul 2, 2021 •

edited

Loading

nicolasburtey Jul 2, 2021

nicolasburtey Jul 2, 2021

nicolasburtey Jul 2, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021 •

edited

Loading

nicolasburtey commented Jul 7, 2021

jrbuhl93 commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

Authentication screen #58

Authentication screen #58

Conversation

jrbuhl93 commented Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrbuhl93 commented Jun 21, 2021 • edited Loading

nicolasburtey commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodymindarts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodymindarts commented Jun 24, 2021

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

jrbuhl93 Jul 2, 2021 • edited Loading

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

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021 • edited Loading

nicolasburtey commented Jul 7, 2021

jrbuhl93 commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

nicolasburtey commented Jul 7, 2021

jrbuhl93 commented Jun 20, 2021 •

edited

Loading

jrbuhl93 commented Jun 21, 2021 •

edited

Loading

jrbuhl93 Jul 2, 2021 •

edited

Loading

nicolasburtey commented Jul 7, 2021 •

edited

Loading