[local_auth] Add Face Unlock support for Android #1940
Conversation
mklim
left a comment
mklim
left a comment
There was a problem hiding this comment.
Would it be feasible to test this at all?
packages/local_auth/README.md
Outdated
| On Android, you can only check for existence of fingerprint hardware prior | ||
| to API 29 (Android Q). Therefore, if you would like to support other biometrics | ||
| types (such as face scanning), *do not* call `getAvailableBiometrics`. Simply call | ||
| `authenticateWithBiometrics`. This will return an error if there was no hardware |
There was a problem hiding this comment.
I think it would be better to have a consistent Dart API here. Instead of asking plugin users to check their SDK and then call plugin APIs differently, could that SDK/Platform check be handled from within the plugin itself so getAvailableBiometrics consistently returns the right data and users don't need to use Error handling for their control flow? From what I can tell SDK 29 should still be able to get this information using hasSystemFeature(FEATURE_FACE), but maybe I'm missing something here? In general it's not unreasonable to case off of SDK versions within the plugin Java code.
There was a problem hiding this comment.
Unfortunately, there are phones out there with face unlock which are on API 27 or 28. While androidx.biometric package works for these, there's no way of knowing that the phones have that hardware without actually instantiating authentication process :(.
The best I can do here is:
- Return a definitive answer for API >= 29
- Return a definitive answer only for fingerprint and a "may be" answer for iris and face unlock for API < 29.
IOW, this API is not usable below 29 if you are interested in non-FP biometrics.
There was a problem hiding this comment.
Ah, I see, thanks for the clarification. :( Strange that androidx.biometric doesn't have any getters when it adds functionality on earlier SDKs.
Even with that in mind, I think it would be cleaner for this to give as much of a definitive answer as it can, so returning FEATURE_FACE on 29 and above. Then this note in the README would only be relevant for callers who specifically want to go beyond and use face unlock < 29.
There was a problem hiding this comment.
Well androidx.biometric is still alpha. Not all functionality is there. 😨
The change you are requesting is reasonable. I will also polish the wording here a bit more.
|
Re: testing. Do we have a testing harness now upstream? I am happy to add more tests. |
|
If we change the Dart API a bit we can definitely unit test that side, but I'm not sure how much we can do with the Java changes. /cc @collinjackson, is our test infra in a place where we could test implementation changes in Android code touching SDK endpoints, like in this PR? |
|
@mklim There is no Dart change in this PR. I would think it is out of scope to change the API and unit test the Dart side. I agree in general we should increase testing but I can't do much in this PR if there's no native testing capability yet. |
|
I am going to submit this since it is blocking an internal team. All review comments have been done except for testing which is currently infeasible for this type of change. |
|
@mehmetf reminding that the policy is to not land PRs that have not been LGTMed. |
|
Just catching up on this. I'm currently focusing on end to end testing as opposed to Roboelectric testing which is what I think you'd want to test this sort of change. I'd like to wire up XCTest mocks and Roboelectric tests on Cirrus (@mklim wrote some for in app purchase) and then we'll be able to ensure that the tests that are contributed are always passing. Relates to |
5 participants
This is not a breaking change.
Only API changed is the
authenticateWithBiometricswhich used to only check for the existence of Fingerprint. Now we support anything the Biometric API supports including face unlock.