Android Support #53

zibs · 2017-09-19T22:38:16Z

This PR adds support for using Fingerprint Authentication on Android.

It keeps the interface mostly the same, aside from allowing the user to pass in a config object to change the title and color of the pop-up requesting authentication.

The error handling is not as robust as iOS, but future PRs would be welcome to improve this.

Readme will be updated with an image showing it working on Android shortly.

I've tested this by creating a new project with react-native init for "react-native": "0.48.1", and it seems to be working for both iOS and Android.

thanks to @sunto for doing the bulk of the work!

Test Plan:

- iOS still works as expected. (no changes were made to any iOS code, so this should be fine)
- Android auth works when isSupported is true
- Calling authenticate when isSupported is false doesn't die.
- Calling isSupported when Android has no fingerprint reader works
- If auth is supported but no fingerprints are enrolled on phone, Android doesn't die.
- If not config object is passed into Android, it defaults to the default config.
- If config object is passed into Android, it uses the default config.
- Android auth works 🎉

Allows for config object to specify title and color of the Fingerprint dialog. Keeps interface mostly the exact same (except for iOS specific error messages). Documentation/README updates to follow in next commits.

naoufal · 2017-09-20T08:48:03Z

👏This is awesome — Great work guys!

🌴On vacation right now, but will try and get to this within a week. Otherwise, I'll see if I can get someone what to help with the review.

zibs · 2017-09-22T21:06:54Z

Sounds good, no rush 👍 - enjoy vacay - I'll do some additional testing this weekend and Sunny will too.

ferrannp · 2017-09-24T15:40:49Z

README.md

+
+On android you will need to add
+```xml
+<uses-permission  android:name="android.permission.USE_FINGERPRINT" />


I think you do not need it. You already add this permission to the manifest library so it will get merged with your manifest (app).

ferrannp · 2017-09-24T15:41:39Z

android/src/main/java/com/rnfingerprint/FingerprintAuthModule.java

+          return false;
+      }
+
+      if (ActivityCompat.checkSelfPermission(getReactApplicationContext(),


You do not need this block, see USE_FINGERPRINT. This is a normal permission so user cannot disable it.

ferrannp · 2017-09-24T15:44:15Z

android/src/main/java/com/rnfingerprint/FingerprintDialog.java

+        super.onCreate(savedInstanceState);
+
+        // Do not create a new Fragment when the Activity is re-created such as orientation changes.
+        setRetainInstance(true);


This is considered a bad practice. You should only retain a fragment for a dialog that returns no view, so onCreateView should return null. Anyway you already use a DialogFragment (they don't leak on rotation), and RN in manifest also says that do not destroy the activity so you do not need this line.

ferrannp · 2017-09-24T15:44:34Z

android/src/main/java/com/rnfingerprint/FingerprintDialog.java

+        setRetainInstance(true);
+        setStyle(DialogFragment.STYLE_NORMAL, android.R.style.Theme_Material_Light_Dialog);
+
+        // dialogCallback = null;


Forgotten comment?

ferrannp · 2017-09-24T15:45:05Z

android/src/main/java/com/rnfingerprint/FingerprintDialog.java

+
+        // Do not create a new Fragment when the Activity is re-created such as orientation changes.
+        setRetainInstance(true);
+        setStyle(DialogFragment.STYLE_NORMAL, android.R.style.Theme_Material_Light_Dialog);


Do we need to set style here explicitly? Can we get the default style? Maybe be able to override it with custom style?

This makes sense, we should probably do something like the following in the TouchID.android.js
DEFAULT_CONFIG = { title: 'Authentication Required', color: '#1306ff', style: 'STYLE_NORMAL' };

ferrannp · 2017-09-24T15:47:14Z

android/src/main/java/com/rnfingerprint/FingerprintHandler.java

+                && mFingerprintManager.hasEnrolledFingerprints()
+                && (ActivityCompat.checkSelfPermission(appContext,
+                        Manifest.permission.USE_FINGERPRINT) ==
+                        PackageManager.PERMISSION_GRANTED);


Same permission comment as before, also, this function seems a duplicated from FingerprintAuthModule.java ?

ferrannp · 2017-09-24T15:47:34Z

android/src/main/java/com/rnfingerprint/FingerprintHandler.java

+
+        if (ActivityCompat.checkSelfPermission(appContext,
+                Manifest.permission.USE_FINGERPRINT) !=
+                PackageManager.PERMISSION_GRANTED) {


Not needed.

See: https://developer.android.com/training/permissions/requesting.html

Normal permissions do not directly risk the user's privacy. If your app lists a normal permission in its manifest, the system grants the permission automatically.

ferrannp · 2017-09-24T15:49:44Z

android/src/main/res/layout/fingerprint_dialog.xml

+                  android:layout_width="49dp"
+                  android:layout_height="49dp"
+                  android:layout_marginLeft="25dp"
+                  android:src="@drawable/ic_fp_40px"


I would not generate a png for this icon but you can generate the xml with Android Studio. Then will look good anywhere.

ferrannp · 2017-09-24T15:50:31Z

android/src/main/res/layout/fingerprint_dialog.xml

+<?xml version="1.0" encoding="utf-8"?>
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:app="http://schemas.android.com/apk/res-auto"
+    android:orientation="vertical" android:layout_width="match_parent"


This file seems it just needs to run 'Reformat' through Android Studio.

ferrannp · 2017-09-24T15:51:31Z

android/build.gradle

+
+  defaultConfig {
+    minSdkVersion 16
+    targetSdkVersion 22


I would bump minSdkVersion and targetSdkVersion up. FingerPrint works only on Android 23 and above, right? So it does not make really sense we say 16.

ferrannp · 2017-09-24T15:54:04Z

@zibs could you update the index.android.js into the examples folder? I actually try it and it works so good so far :). Just added some comments into the PR.

@naoufal maybe you want to change the lib name ! And support face id for iOS in the future ;) Just saying.

wireforce · 2017-09-30T09:31:46Z

Maybe rename to react-native-biometric-auth ? :)

zibs · 2017-09-30T17:38:44Z

@ferrannp Thanks so much for the review! Will take a look at get these updates to you shortly.

naoufal · 2017-10-02T15:58:03Z

@zibs I dig it, picked up https://www.npmjs.com/package/react-native-biometric-auth. Will add you and @sunto to the package.

GantMan · 2017-10-18T20:54:05Z

Excited to see this land!

zibs · 2017-10-18T21:15:02Z

landing shortly...thanks for the nudge @GantMan 🥇

…tive

Android: possible to use all color formats that are valid in react-na…

harrisrobin · 2017-10-30T14:41:14Z

@zibs we're super excited to see this land! Great work guys :)

tactico · 2017-10-30T14:42:19Z

@zibs Also looking forward to this!

ben-snaize · 2017-11-03T12:06:45Z

Is it in yet? Hyped for this!

zibs · 2017-11-03T14:52:37Z

Sorry for delay all - just got one or two last things to update here and we should be good!

tactico · 2017-11-03T16:05:06Z

I could not wait and implemented your branch and started testing. One comment would be to synchronize the error codes with the iOS version, for example in the IOS version I get an error of the form "LAErrorUserCancel" and the same error in this branch comes through as "Touch ID Error"

zibs · 2017-11-03T17:40:22Z

@tactico Feel free to make a PR! The errors are handled fundamentally different on iOS and Android, which is why I haven't gone down that path yet. Definitely could be somewhat integrated eventually though!

naoufal · 2017-11-06T16:52:04Z

Agreed, let's tack on error handling in a subsequent PR, since even the current iOS error handling could use some work. @tactico, let me know if you're up for taking this on.

…

On Fri, Nov 3, 2017, 10:40 AM Eli Zibin ***@***.***> wrote: @tactico <https://github.com/tactico> Feel free to make a PR! The errors are handled fundamentally different on iOS and Android, which is why I haven't gone down that path yet. Definitely could be somewhat integrated eventually though! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#53 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABjWsLU51DqtuKBlwATSRQUAbfPY0jiWks5sy1AGgaJpZM4PdG6w> .

tactico · 2017-11-07T01:30:18Z

I probably will not be able to get to it for about a week, major release of our product scheduled for this Friday!

tactico · 2017-11-14T20:50:13Z

Sorry noufal, got pulled into another project so likely will not be able to do this anytime soon. My apologies...

zibs · 2017-11-21T17:49:24Z

@naoufal Hey Naoufal, sorry for dropping the ball and taking so long on this one. I've just pushed a number of commits that addressed most of the issues in the review. There is one missing about passing in a custom style theme thing, but I think that can wait until later!

I just went through and tested on a brand new react native project created with react-native init - I wasn't able to run the example cause the yarn.lock is pointing to netflix artifacts 🎥 . But I added the example code I used in my test run and it should be all good!

Let me know if you have any suggestions!

naoufal · 2017-11-21T17:55:51Z

No worries at all! This is great work that a bunch of people will benefit from, I'd say you're doing the total opposite of dropping the ball! 🍻

Sorry about the netflix artifacts -- I'll fix that!

I'll take a look tonight/tomorrow evening and try and get us setup for a merge this weekend.

naoufal · 2017-12-03T04:07:45Z

@zibs updated the yarn.lock. Also merged the FaceID PR so you'll need to update the README. Once that's done hit the merge button!

And I'll bake this into the next release 🎉

fretman92 · 2018-06-11T19:57:38Z

TouchID.android.js

+  },
+
+  authenticate(reason, config) {
+    DEFAULT_CONFIG = { title: 'Authentication Required', color: '#1306ff' };


why do you not declare the DEFAULT_CONFIG variable here?
It causes error in strict mode.

Eli Zibin added 5 commits September 14, 2017 10:48

Adds experimental Android Support:

ab0ea7e

Allows for config object to specify title and color of the Fingerprint dialog. Keeps interface mostly the exact same (except for iOS specific error messages). Documentation/README updates to follow in next commits.

Update readme with Android usage

5b3ffed

Update Readme.

fa3aedd

add permissions note for android to readme

e72c01c

typo

d763d0d

zibs mentioned this pull request Sep 19, 2017

Make API surface consistent for android #52

Closed

ferrannp reviewed Sep 24, 2017

View reviewed changes

begin making changes from PR review

796eb1d

Eli Zibin and others added 3 commits October 27, 2017 10:39

reformat fingerprint_dialog.xml

412cda3

Android: possible to use all color formats that are valid in react-na…

a575551

…tive

Merge pull request #2 from mikaello/fix-all-color-issue

e0619bb

Android: possible to use all color formats that are valid in react-na…

Eli Zibin added 2 commits November 21, 2017 09:23

relax minSDK version to play nicely with RN

18f4378

fix issue where key was getting set on immutable object

ecfb0f9

update android example

020f6c3

Merge branch 'master' into master

9dcab28

Merge branch 'master' into master

d682095

zibs merged commit 6e47184 into naoufal:master Dec 3, 2017

fretman92 reviewed Jun 11, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android Support #53

Android Support #53

zibs commented Sep 19, 2017 •

edited

Loading

naoufal commented Sep 20, 2017

zibs commented Sep 22, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

sunto Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp Sep 24, 2017

ferrannp commented Sep 24, 2017

wireforce commented Sep 30, 2017

zibs commented Sep 30, 2017

naoufal commented Oct 2, 2017

GantMan commented Oct 18, 2017

zibs commented Oct 18, 2017

harrisrobin commented Oct 30, 2017

tactico commented Oct 30, 2017

ben-snaize commented Nov 3, 2017

zibs commented Nov 3, 2017 •

edited

Loading

tactico commented Nov 3, 2017

zibs commented Nov 3, 2017

naoufal commented Nov 6, 2017 via email

tactico commented Nov 7, 2017

tactico commented Nov 14, 2017

zibs commented Nov 21, 2017 •

edited by naoufal

Loading

naoufal commented Nov 21, 2017

naoufal commented Dec 3, 2017

fretman92 Jun 11, 2018 •

edited

Loading

Android Support #53

Android Support #53

Conversation

zibs commented Sep 19, 2017 • edited Loading

Test Plan:

naoufal commented Sep 20, 2017

zibs commented Sep 22, 2017

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

ferrannp commented Sep 24, 2017

wireforce commented Sep 30, 2017

zibs commented Sep 30, 2017

naoufal commented Oct 2, 2017

GantMan commented Oct 18, 2017

zibs commented Oct 18, 2017

harrisrobin commented Oct 30, 2017

tactico commented Oct 30, 2017

ben-snaize commented Nov 3, 2017

zibs commented Nov 3, 2017 • edited Loading

tactico commented Nov 3, 2017

zibs commented Nov 3, 2017

naoufal commented Nov 6, 2017 via email

tactico commented Nov 7, 2017

tactico commented Nov 14, 2017

zibs commented Nov 21, 2017 • edited by naoufal Loading

naoufal commented Nov 21, 2017

naoufal commented Dec 3, 2017

fretman92 Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

zibs commented Sep 19, 2017 •

edited

Loading

zibs commented Nov 3, 2017 •

edited

Loading

zibs commented Nov 21, 2017 •

edited by naoufal

Loading

fretman92 Jun 11, 2018 •

edited

Loading