Skip to content

Added biometric support api#2775

Closed
Batlin wants to merge 6 commits into
owncloud:masterfrom
Batlin:feature/biometric_api
Closed

Added biometric support api#2775
Batlin wants to merge 6 commits into
owncloud:masterfrom
Batlin:feature/biometric_api

Conversation

@Batlin
Copy link
Copy Markdown
Contributor

@Batlin Batlin commented Dec 29, 2019

Replace custom fingerprint authentication with androidx biometric library (supports both fingerprint and face unlock mechanisms). This library makes all the features announced in Android 10 (API level 29) available all the way back to Android 6 (API level 23).

New strings (shown at the biometric prompt) are available in english and spanish. Rest of languages are missing (strings biometric_prompt_title and biometric_prompt_subtitle).

Screenshot_20191229-105821


QA

Test plan:

https://github.com/owncloud/QA/blob/master/Mobile/Android/Release_2.15/2775-biometric_support.md

BUGS & IMPROVEMENTS:

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Jan 7, 2020

Thanks for the contribution!! we will dive into it and let you know. Since the contribution is marked as your first contribution, i will give a couple of tips about our process:

  • Your contribution will be triaged to be included in one of our sprints (depending priorities etc...)
  • The mobile team will check your code, will review it, and maybe you will be suggested changes (maybe not), to assure the code quality as much as posible.
  • Once the code review is passed, a QA - test stage will start, to be sure it works as expected. So, you can be reported bugs! (i hope no!)
  • Finally, it will be merged for the next product version.

Thanks again!!! i hope this is only the first one of many contributions ;)

@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Jan 7, 2020

Thank you very much Jesús and happy new year all!

I'd really love to see this being included soon in the production apk. I'm ready to submit any needed modification to the PR (although I hope everything is ok!).

Best regards,

@abelgardep
Copy link
Copy Markdown
Contributor

Pretty cool feature, we will take a look as soon as possible 👍

@jesmrec jesmrec added this to the 2.15 milestone Jan 8, 2020
Copy link
Copy Markdown
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

Some changes requested @Batlin . Thanks for the contribution, looks good 👍

Comment thread build.gradle
Comment thread owncloudApp/src/main/java/com/owncloud/android/ui/activity/BiometricActivity.java Outdated
Comment thread owncloudApp/src/main/res/xml/preferences.xml Outdated
Comment thread owncloudApp/src/main/java/com/owncloud/android/ui/activity/BiometricActivity.java Outdated
Comment thread owncloudApp/src/main/java/com/owncloud/android/ui/activity/BiometricActivity.java Outdated
Comment thread owncloudApp/src/main/java/com/owncloud/android/ui/activity/BiometricActivity.java Outdated
Comment thread owncloudApp/src/main/java/com/owncloud/android/ui/activity/BiometricActivity.java Outdated
@Batlin Batlin requested a review from abelgardep January 14, 2020 17:57
@Batlin Batlin force-pushed the feature/biometric_api branch from 6342dd4 to 1cc26a9 Compare January 14, 2020 18:47
@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Jan 14, 2020

Thanks @abelgardep for your review. I'm really excited to see this moving along! :)

I've just submitted all suggested changes in a new commit (and made a rebase with master branch), except the ones involving the "set_fingerprint" key. I wanted to confirm with you before committing, because if we rename that key, I think people will have to configure that setting again after updating their apk. Please, confirm me if I should submit a new commit including that change.

Thanks again for your time. Best regards,

@abelgardep abelgardep requested a review from davigonz January 15, 2020 08:19
@jesmrec jesmrec added the Sprint label Jan 20, 2020
@davigonz
Copy link
Copy Markdown
Contributor

if we rename that key, I think people will have to configure that setting again after updating their apk

@Batlin well, it's not trivial but you could try the following:

  1. From registerActivityLifecycleCallbacks, onActivityCreated method in MainApp.kt you could call a static method created in PreferenceManager.java which will delete the set_fingerprint key from preferences (check if it exists before) and create a new set_biometric key with the same value as set_fingerprint.
  2. You will also need to change the set_fingerprint key in preferences.xml to set_biometric.

We will need to keep that code to make sure we do not break the app for those users upgrading from older versions to new ones.

Try it out and tell me.

@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Jan 22, 2020

Ok, I will try it and push a new commit next Friday.

Thank you.

@Batlin Batlin force-pushed the feature/biometric_api branch from 1cc26a9 to 61fe2ea Compare January 24, 2020 18:01
@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Jan 24, 2020

I've just implemented and tested it. You can check the new commit (I've rebased with master also).

Thank you, hope to see this included soon!

@davigonz
Copy link
Copy Markdown
Contributor

I've just implemented and tested it. You can check the new commit (I've rebased with master also).
Thank you, hope to see this included soon!

Thanks @Batlin , having a look at it

@davigonz
Copy link
Copy Markdown
Contributor

davigonz commented Jan 28, 2020

@Batlin I've requested a few more changes and we are ready to test the feature

@Batlin Batlin force-pushed the feature/biometric_api branch from 61fe2ea to 3e2a211 Compare February 2, 2020 09:59
@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Feb 2, 2020

@davigonz, I've just rebased against master (no conflicts) and pushed the requested changes.

Just to let you know, depending on the UX you want to provide to end users, we could customize the BiometricPrompt that is created in BiometricActivity changing the setConfirmationRequired method. As it's set to true now there is a confirmation required (user has to click on Confirm button) after a biometric has been authenticated. If we set it to false, we could provide a faster authentication experience.

Thank you.

@Batlin Batlin requested a review from davigonz February 2, 2020 10:06
@davigonz
Copy link
Copy Markdown
Contributor

davigonz commented Feb 3, 2020

@davigonz, I've just rebased against master (no conflicts) and pushed the requested changes.

Just to let you know, depending on the UX you want to provide to end users, we could customize the BiometricPrompt that is created in BiometricActivity changing the setConfirmationRequired method. As it's set to true now there is a confirmation required (user has to click on Confirm button) after a biometric has been authenticated. If we set it to false, we could provide a faster authentication experience.

Thank you.

Thanks a lot for applying the changes, the code looks good to me, now it's the turn of @jesmrec who will perform some QA tests.

About the faster authentication experience while using the feature, I'm comparing it with the current one

@davigonz
Copy link
Copy Markdown
Contributor

davigonz commented Feb 3, 2020

As it's set to true now there is a confirmation required (user has to click on Confirm button) after a biometric has been authenticated. If we set it to false, we could provide a faster authentication experience.

@Batlin I do not see that confirmation button, where should it appear? I use my fingerprint in the app and the filelist directly appears without any confirmation button.

@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Feb 3, 2020

@davigonz, you are right, I've just tested this on a Xiaomi Mi 9T and there is no confirmation button using fingerprint authentication.

But you can see the difference with the face unlock on Pixel 4 (with setConfirmationRequired set to true):

image

And setConfirmationRequired set to false:

image

Best regards,

@davigonz
Copy link
Copy Markdown
Contributor

davigonz commented Feb 4, 2020

But you can see the difference with the face unlock on Pixel 4 (with setConfirmationRequired set to true)

Ok, thanks @Batlin !

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Feb 5, 2020

Let's start the QA stage.

In the first message i've added a link to the test plan and i will add the bugs, improvements etc i'd find out

@davigonz
Copy link
Copy Markdown
Contributor

Hi @davigonz, @jesmrec,

I've just pushed a new commit including the FLAG_ACTIVITY_SINGLE_TOP, and rebased with master.

I hope it gets merged soon!

Best regards,

Thanks a lot @Batlin , I guess @jesmrec can resume the QA work here, we will keep you updated.

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Feb 17, 2020

bug of double passcode is fixed.

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Feb 21, 2020

I did a couple of tests with a Samsung device with Android 9, which contains its own biometrical unlock implementation. By enrolling face and fingerprint is always asked fingerprint, no matter the preference you set.

@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Mar 1, 2020

Any news on this PR? Which are the next steps?

Best regards,

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 2, 2020

tomorrow we will test with real device Android 10 to check if everyhing is finally OK

@michaelstingl
Copy link
Copy Markdown
Contributor

Everything worked fine on my Google Pixel 4, but the text always says "Fingerprint", and the Pixel 4 only has FaceID.

Android version: 10
Security patch level: 5 March 2020

2020-03-03 16 09 58

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 3, 2020

@Batlin could you take a look? this is the last remaining issue before merging.

With a Pixel2 upgraded to Android10, this is how it looks like that is more correct:

Screenshot_20200303-162939

any difference in the new device that make the difference?

both screenshots were taken using the same commit.

@Batlin Batlin force-pushed the feature/biometric_api branch from ec2b0f2 to 8add1b7 Compare March 4, 2020 19:49
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Mar 4, 2020

@jesmrec, I've just rebased the branch, without conflicts, and pushed a new commit to fix the reported string issue (really hope it gets merged!).

The only possible explanation is that @michaelstingl has set English (from United Kingdom) in his device and, therefore, it's using strings.xml from values-en-rGB.

Please, keep in mind as I have already warned in my first post that new strings (shown at the biometric prompt) are only available in english (now both US and UK) and spanish. Rest of languages are missing (strings biometric_prompt_title and biometric_prompt_subtitle will be shown in english if users have a different language configured). prefs_biometric string ("fingerprint lock") should be modified in other languages as well.

Thank you all.

Best regards,

@davigonz davigonz closed this Mar 5, 2020
@davigonz davigonz reopened this Mar 5, 2020
@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 5, 2020

Thanks @Batlin , your explanation makes much sense. @michaelstingl can check and we approve then if everything is in order.

Let's also check why CI is not passing in this PR.

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 5, 2020

Other issue to take in account are the UI tests.

In settings sets, we used to use the string resource R.string.prefs_fingerprint that has to be changed for R.string.prefs_biometric in

https://github.com/owncloud/android/blob/master/owncloudApp/src/androidTest/java/com/owncloud/android/settings/security/OCSettingsSecurityTest.kt

otherwise, tests will fail.

@michaelstingl
Copy link
Copy Markdown
Contributor

Good point with the languages…

English (Germany) Arabic German English (UK)
2020-03-05 11 12 50 2020-03-05 11 13 22 2020-03-05 11 13 57 2020-03-05 11 14 34
2020-03-05 11 13 03 2020-03-05 11 13 41 2020-03-05 11 14 13 2020-03-05 11 14 46
Biometric Fingerprint Fingerprint Biometric

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 5, 2020

Let's then transifex do its magic... i will add by myself the spanish translation :D

From QA this is approved. We have to deal the CI (why the hell is it stucked?) and the UI tests due to the change of resource name.

Thanks for your help @michaelstingl !!

@Batlin
Copy link
Copy Markdown
Contributor Author

Batlin commented Mar 6, 2020

Should I push a new commit with the UI test modification?

Is there anything else I should do? Just let me know.

Thank you!

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 9, 2020

It would be awesome if you put the commit with the wording modification @Batlin. Thanks!

@jesmrec
Copy link
Copy Markdown
Contributor

jesmrec commented Mar 12, 2020

Finally, these were the actions:

  • Due to the CI problems, i moved your commits to a new PR: Added biometric support api II #2840

  • UI tests were fixed with an additional commit.

  • PR was merged and new feature will be included in 2.15

Thanks again for your patience and engagement @Batlin!

@abelgardep
Copy link
Copy Markdown
Contributor

Merged via #2840, thanks @Batlin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants