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

Use biometricpromt and more consistent callbacks #179

Conversation

greaterking
Copy link
Collaborator

@greaterking greaterking commented Aug 17, 2019

Everything seems to be working but before moving on further wanted you to review some of the changes. Haven't had time to do much with language support as promised ..but this can be easily done the aar file is just a module export from a lib I've refactored for the purposes of this plugin. I've tested pretty extensively. Face detection even works on android.

Would be nice if...

  1. there was a way to determine what biometric type is available for use on android (I've looked haven't found a reliable way)
  2. if there was a way to know what lock type is set on android (I've looked haven't found a reliable way)
    ...but these bridges can be crossed later.

Description

  • made description and subtitle optional in android if null is passed they won't show
  • removed onAuthenticationFail even since its better suited for use with observable not promise (can add back anytime if we feel that reported every failed attempt of a scan is needed. Most people just want success or do X when sensor is locked-out which the plugin already does)
  • spelling errors fixed in tests.js
  • wrote new test and adjusted current ones
  • updating js interface to accommodate all new parameters
  • updated readme outlining new changes in v3
  • updated aar to remove unused events
  • updated ionic native wrapper (unpublished atm)
  • removed res folder
  • bumped version

How did you test your changes?

ios sim, android sim, ios 7 device, android galaxy s9 device

Closes #129 #198 #158 #83 #151

greaterking and others added 16 commits August 6, 2019 19:11
- upgrading fingerprint to use biometric prompt
- renamed packages
…me old things

- removed all res/ references
…me old things

- removed all res/ references
…ent-callbacks' into 2.0.0-biometricpromt-and-consistent-callbacks
- isAvailable can actually fail
…ent-callbacks' into 2.0.0-biometricpromt-and-consistent-callbacks
… they won't show

- removed onAuthenticationFail even since its better suited for use with observable not promise (can add back anytime if we feel that reported every failed attempt of a scan is needed. Most people just want success or do X when sensor is locked-out which the plugin already does)
- spelling errors fixed in tests.js
- wrote new test and adjusted current ones
- updating js interface to accommodate all new parameters
- updated readme outlining new changes in v3
- updated aar to remove unused events
- updated ionic native wrapper (unpublished atm)
- bumped version
@greaterking greaterking marked this pull request as ready for review August 17, 2019 04:02
@NiklasMerz
Copy link
Owner

Thank you very much. I love this PR, because the Android part is much more readable than before.

I need to do some tests but reading the code for the first time looks good to me. Could you please resolve the conflicts as well? I don't want to break it :-)

there was a way to determine what biometric type is available for use on android (I've looked haven't found a reliable way)

That would be good since it is often used on iOS. I will look for an solution as well.

Haven't had time to do much with language support as promised ..but this can be easily done the aar file is just a module export from a lib I've refactored for the purposes of this plugin

Is the language support for the native dialogue necessary like before? How does it work now? I don't know that much about that?

What is that aar file? Where is the code for that? How is that licensed. Since you wrote the Android part from scratch I would like to change the licensing to MIT for all code. The previous version contained some Apache licensed code from the other Android plugin.

@NiklasMerz NiklasMerz added the release:major Candidate for next major release label Aug 17, 2019
@NiklasMerz NiklasMerz added this to the 3.0 milestone Aug 17, 2019
@NiklasMerz
Copy link
Owner

I resolved the conflict myself to get the CI build runnin. I hope I did not mess up.

What about the SDK version check from the old version?

JSONObject errorResponse = new JSONObject();

        if (android.os.Build.VERSION.SDK_INT < 23) {
            Log.e(TAG, "minimum SDK version 23 required");
            mPluginResult = new PluginResult(PluginResult.Status.ERROR);
            mCallbackContext.error(errorResponse.put("message", "minimum SDK version 23 required"));
            mCallbackContext.sendPluginResult(mPluginResult);
            return true;
        }

Should we add this back?

@NiklasMerz NiklasMerz changed the title 3.0.0 biometricpromt and consistent callbacks Use biometricpromt and more consistent callbacks Aug 17, 2019
Copy link
Owner

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's dicuss the oustanding questions a do some tests.

@greaterking
Copy link
Collaborator Author

greaterking commented Aug 17, 2019

You resolved the conflicts without issue sorry I missed this ..I didn't think I had the right permissions ..first time doing the draft-pr here.

Is the language support for the native dialogue necessary like before? How does it work now? I don't know that much about that?

I don't think it is. In iOS for example the only things you can dynamically change in the prompt are the description[localizedReason](text that appears under the title) and the fallback button title[localizedFallbackTitle](2nd button that appears after first failed fingerprint auth). The title and everything else already would support whatever languages iOS already supports. Since the new plugin returns error codes now for every error users should be able to customize the results or error messages to their liking. In android you can set everything; title, subtitle, description ..so no I don't think its needed anymore.

What is that aar file? Where is the code for that? How is that licensed. Since you wrote the Android part from scratch I would like to change the licensing to MIT for all code. The previous version contained some Apache licensed code from the other Android plugin.

The aar file is just an exported android lib I created a module from using sample code from https://github.com/anitaa1990/Biometric-Auth-Sample according to jcenter according to the libs repository jcenter this uses apache 2.0 liscene which you already have commented in Fingerprint.java file. The apache license should be fine unless there is a difference I'm not aware of.

Additionally my approach here was to simply and unify things as much as I could. This plugin is great and now the shelf life has increase some.

I know a lot of ionic(angular) users use this plugin as well so we'll have to update the examples codes under the How to use section. I've taken the liberty to also work on the wrapper which I've finished updating but haven't made a PR with ionic-native yet.

  • For android the secret key and client id were no longer needed as this done dynamically using UUID.randomUUID().toString()
  • isAvailable method also checks for minimum sdk requirements via it's onSdkVersionNotSupported() event and will report back with that error if the check fails.
    Internally looks like this
   /*
     * Condition I: Check if the android version in device is greater than
     * Marshmallow, since fingerprint authentication is only supported
     * from Android 6.0.
     * Note: If your project's minSdkversion is 23 or higher,
     * then you won't need to perform this check.
     *
     * */
    public static boolean isSdkVersionSupported() {
        return (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M);
    }

Please let me know if you have anymore questions.

@NiklasMerz
Copy link
Owner

You resolved the conflicts without issue sorry I missed this ..I didn't think I had the right permissions ..first time doing the draft-pr here.

You created a full PR but everything is fine. I can push to it as well, so thats perfect

Is the language support for the native dialogue necessary like before? How does it work now? I don't know that much about that?

I don't think it is. In iOS for example the only things you can dynamically change in the prompt are the description[localizedReason](text that appears under the title) and the fallback button title[localizedFallbackTitle](2nd button that appears after first failed fingerprint auth). The title and everything else already would support whatever languages iOS already supports. Since the new plugin returns error codes now for every error users should be able to customize the results or error messages to their liking. In android you can set everything; title, subtitle, description ..so no I don't think its needed anymore.

That's good. Adding a new file for every new language was kind of silly. I will test it.

What is that aar file? Where is the code for that? How is that licensed. Since you wrote the Android part from scratch I would like to change the licensing to MIT for all code. The previous version contained some Apache licensed code from the other Android plugin.

The aar file is just an exported android lib I created a module from using sample code from https://github.com/anitaa1990/Biometric-Auth-Sample according to jcenter according to the libs repository jcenter this uses apache 2.0 liscene which you already have commented in Fingerprint.java file. The apache license should be fine unless there is a difference I'm not aware of.

Apache License is fine but we should a note about this library with a link to it and its license to readme. Add it to the credits at the end.

So you have module project which build the aar file, correct? Should we add this code to the plugin? Or you could create a fork of the original code, add your changes and a link. We need to have this code available and properly attributed and licensed.

Additionally my approach here was to simply and unify things as much as I could. This plugin is great and now the shelf life has increase some.

I love how much simpler this is than before. The key handling stuff was not great.

I know a lot of ionic(angular) users use this plugin as well so we'll have to update the examples codes under the How to use section. I've taken the liberty to also work on the wrapper which I've finished updating but haven't made a PR with ionic-native yet.

I agree, Updating the docs is important. The README is correct in this PR? Yes Ionic native and its docs need to be updated when this is released.

* For android the secret key and client id were no longer needed as this done dynamically using `UUID.randomUUID().toString()`

* **isAvailable** method also checks for minimum sdk requirements via it's `onSdkVersionNotSupported()` event and will report back with that error if the check fails.
  Internally looks like this
   /*
     * Condition I: Check if the android version in device is greater than
     * Marshmallow, since fingerprint authentication is only supported
     * from Android 6.0.
     * Note: If your project's minSdkversion is 23 or higher,
     * then you won't need to perform this check.
     *
     * */
    public static boolean isSdkVersionSupported() {
        return (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M);
    }

Great. Sorry I missed that.

Please let me know if you have anymore questions.

I still don't know how much testing this needs. Is backwartscompatibilty fine? I need to set some time to check everything. Since I cannot tests everything I need some help I guess. We'll figure that out.

Thanks again. I hope you are patient :-)

@greaterking
Copy link
Collaborator Author

@greaterking Great! Do you plan a PR for ionic native when this is released?

Yes I plan on and following their guide for pull request as the code is already updated and complete.

What can we do about that cancel button?

In my app I haven't seen this cancel issue yet..so trying to reporduce will run the same test as you via npm and will report back.

@greaterking
Copy link
Collaborator Author

greaterking commented Nov 2, 2019

I'm trying to run the test but getting this as an error

npm run test-android
> npx cordova-paramedic --platform android --plugin  $(pwd)

npx: installed 62 in 2.676s
cordova-paramedic: creating temp project at /var/folders/3m/8t6zmbwj2yv8jdmq20th67nw0000gn/T/tmp-6459364iwSy2jelPG
cordova-paramedic: installing /Users/****/Sites/ura-app-v3/plugins/cordova-plugin-fingerprint-aio
cordova-paramedic: installing /Users/***/Sites/ura-app-v3/plugins/cordova-plugin-fingerprint-aio/tests
cordova-paramedic: installing plugin-test-framework
cordova-paramedic: starting local medic server android
(node:64593) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
cordova-paramedic: writing medic log url to project
cordova-paramedic: setting app start page to test page
cordova-paramedic: adding platform
This test seems to be blocked :: timeout exceeded. Exiting ...
//LOG
0 info it worked if it ends with ok
1 verbose cli [
1 verbose cli   '/usr/local/bin/node',
1 verbose cli   '/Users/*******/.npm-global/lib/node_modules/npm/bin/npm-cli.js',
1 verbose cli   'run',
1 verbose cli   'test-android',
1 verbose cli   '--scripts-prepend-node-path=auto'
1 verbose cli ]
2 info using [email protected]
3 info using [email protected]
4 verbose run-script [ 'pretest-android', 'test-android', 'posttest-android' ]
5 info lifecycle [email protected]~pretest-android: [email protected]
6 info lifecycle [email protected]~test-android: [email protected]
7 verbose lifecycle [email protected]~test-android: unsafe-perm in lifecycle true
8 verbose lifecycle [email protected]~test-android: PATH: /Users/*******/.npm-global/lib/node_modules/npm/node_modules/npm-lifecycle/node-gyp-bin:/Users/*******/Sites/ura-app-v3/plugins/cordova-plugin-fingerprint-aio/node_modules/.bin:/opt/local/bin:/opt/local/sbin:/Users/*******/.npm-global/bin:/Users/*******/Library/Android/sdk/platform-tools:/Users/*******/Library/Android/sdk/tools:/Applications/XAMPP/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/*******/.composer/vendor/bin:/usr/local/opt/gradle/libexec:/Users/*******/Library/Android/sdk/platform-tools:/Users/*******/Library/Android/sdk/tools:/Applications/XAMPP/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/*******/.composer/vendor/bin:/Users/*******/.npm-global/bin
9 verbose lifecycle [email protected]~test-android: CWD: /Users/*******/Sites/ura-app-v3/plugins/cordova-plugin-fingerprint-aio
10 silly lifecycle [email protected]~test-android: Args: [ '-c', 'npx cordova-paramedic --platform android --plugin  $(pwd)' ]
11 silly lifecycle [email protected]~test-android: Returned: code: 1  signal: null
12 info lifecycle [email protected]~test-android: Failed to exec test-android script
13 verbose stack Error: [email protected] test-android: `npx cordova-paramedic --platform android --plugin  $(pwd)`
13 verbose stack Exit status 1
13 verbose stack     at EventEmitter.<anonymous> (/Users/*******/.npm-global/lib/node_modules/npm/node_modules/npm-lifecycle/index.js:326:16)
13 verbose stack     at EventEmitter.emit (events.js:210:5)
13 verbose stack     at ChildProcess.<anonymous> (/Users/*******/.npm-global/lib/node_modules/npm/node_modules/npm-lifecycle/lib/spawn.js:55:14)
13 verbose stack     at ChildProcess.emit (events.js:210:5)
13 verbose stack     at maybeClose (internal/child_process.js:1021:16)
13 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)
14 verbose pkgid [email protected]
15 verbose cwd /Users/*******/Sites/ura-app-v3/plugins/cordova-plugin-fingerprint-aio
16 verbose Darwin 18.7.0
17 verbose argv "/usr/local/bin/node" "/Users/*******/.npm-global/lib/node_modules/npm/bin/npm-cli.js" "run" "test-android" "--scripts-prepend-node-path=auto"
18 verbose node v12.13.0
19 verbose npm  v6.10.2
20 error code ELIFECYCLE
21 error errno 1
22 error [email protected] test-android: `npx cordova-paramedic --platform android --plugin  $(pwd)`
22 error Exit status 1
23 error Failed at the [email protected] test-android script.
23 error This is probably not a problem with npm. There is likely additional logging output above.
24 verbose exit [ 1, true ]

probably something on my end...will test more tomrr

@paulstelzer
Copy link

@greaterking @NiklasMerz I can help with the ionic native wrapper because I am member of the ionic team, a PR is very easy to create at Ionic native

@NiklasMerz
Copy link
Owner

NiklasMerz commented Nov 2, 2019 via email

Copy link
Owner

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make this cancel button clearer

src/android/BiometricActivity.java Outdated Show resolved Hide resolved
src/android/PromptInfo.java Outdated Show resolved Hide resolved
@Qsaws
Copy link

Qsaws commented Nov 7, 2019

Is there a way to get the list of available biometrics systems identifications on the device? (finger and face for exemple). And specify in the show() method which system to use?

isAvailable() returns only one of them right now and we want to let the user pick between the two if he has both choices available on his device.

Thank you and sorry if this isn't the right place.

@exxbrain
Copy link
Collaborator

exxbrain commented Nov 11, 2019

Is there a way to get the list of available biometrics systems identifications on the device? (finger and face for exemple). And specify in the show() method which system to use?

isAvailable() returns only one of them right now and we want to let the user pick between the two if he has both choices available on his device.

@Qsaws Biometric auth works another way. User selects a kind of biometric in a device settings. It will be used to authenticate in apps. The selection of biometric is not accessible in an app itself. Application just provides an ability to use device-level selected biometric. So actually you don't need a result of isAvailable().

@Qsaws
Copy link

Qsaws commented Nov 11, 2019

Is there a way to get the list of available biometrics systems identifications on the device? (finger and face for exemple). And specify in the show() method which system to use?
isAvailable() returns only one of them right now and we want to let the user pick between the two if he has both choices available on his device.

@Qsaws Biometric auth works another way. User selects a kind of biometric in a device settings. It will be used to authenticate in apps. The selection of biometric is not accessible in an app itself. Application just provides an ability to use device-level selected biometric. So actually you don't need a result of isAvailable().

Ah i see, thanks for the explanation ;) .

@richardshergold
Copy link

@NiklasMerz do you have an approximate release date for the new version of this plugin?

@NiklasMerz
Copy link
Owner

Any objections against a new release?

Version 3.0.1 may follow soon enough :-)

@greaterking
Copy link
Collaborator Author

Any objections against a new release?

I'm ready on my end to make pull request to ionic/native

@NiklasMerz
Copy link
Owner

Any objections against a new release?

I'm ready on my end to make pull request to ionic/native

Did you already add the new parameter cancelButtonTitle? It is needed for Android.

@NiklasMerz
Copy link
Owner

Let's go. Thank you very much everyone here for testing, development and you patience. I will do a new release now! This would not be possible without @greaterking and @exxbrain !

@NiklasMerz NiklasMerz merged commit 67e877d into NiklasMerz:master Nov 12, 2019
@greaterking
Copy link
Collaborator Author

Any objections against a new release?

I'm ready on my end to make pull request to ionic/native

Did you already add the new parameter cancelButtonTitle? It is needed for Android.
doing this now...thanks!

@NiklasMerz
Copy link
Owner

The release is done! Looking forward to the Ionic Native integration.

Just look at the number of very old issues closed with this PR
grafik

I would love to people involved here to stick around maintaining this plugin. I am open to add new maintainers to this project. Especially @greaterking and @exxbrain you know a lot more about these Android APIs than I know. Please help me out if you can :-)

A lot of the work that has been around for some time is done now, but a little bit is left. I think about #176 and real AndroidX support.

@greaterking
Copy link
Collaborator Author

greaterking commented Nov 12, 2019

Thank you to @NiklasMerz you too sir for this great plugin!
Ionic native pull request can be found here: danielsogl/awesome-cordova-plugins#3227
will for sure be here to help test/implement android x, biometric types. #176 most likely will land in 3.0.1 thanks for patience in all things.

@NiklasMerz
Copy link
Owner

Just a side not: I wrote a blog post about this plugin and PR.

@greaterking and @exxbrain you are the heroes of this story

@greaterking
Copy link
Collaborator Author

greaterking commented Dec 4, 2019 via email

@Sumyth7
Copy link

Sumyth7 commented Dec 5, 2019 via email

@NiklasMerz
Copy link
Owner

@Sumyth7 I do not know how Samsung devices and their Iris scanner work. Please create a separate issue and explain your problem more. Probably someone else here can say more about Samsung?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android ios release:major Candidate for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FingerprintManager deprecated: Plans to use BiometricPrompt?
9 participants