[local_auth] Fix getEnrolledBiometrics returning non-enrolled biometrics on Android. #5309
Conversation
| if (packageManager.hasSystemFeature(PackageManager.FEATURE_IRIS)) { | ||
| biometrics.add("iris"); | ||
| } | ||
| // If no biometrics are enrolled, only "undefined" is returned. |
There was a problem hiding this comment.
Since this is a bizarre implementation anyway, and you're already adjusting the method channel slightly, let's just split out a separate method channel call that's backed by canAuthenticateWithBiometrics. Then this method can only return enrolled biometrics, eliminating "undefined", and the Dart code that currently relies on this special value can instead use that new method channel call directly.
There was a problem hiding this comment.
In the current implementation, the undefined value gets ignored on the dart side, both for Android and iOS. As such, I don't believe there is any dart logic that is currently depending on this value, it's just a weird detail being sent back by the native iOS implementation.
Should we just consider removing undefined alltogether in both implementations instead?
Additionally, we could add a second method to the interface (e.g. getPresentBiometrics) that just returns a list of all present biometric types, disregarding wether they are enrolled or not, so that this functionality remains available on Android. (But this would then need to also be implemented on iOS)
There was a problem hiding this comment.
In the current implementation, the
undefinedvalue gets ignored on the dart side, both for Android and iOS. As such, I don't believe there is any dart logic that is currently depending on this value, it's just a weird detail being sent back by the native iOS implementation.
It's not ignored, it's how this works.
Should we just consider removing
undefinedalltogether in both implementations instead?
Yes, I'm suggesting that we remove "undefined" in favor of a new platform channel method that allows direct querying of whether the device supports biometrics. I.e., have a deviceSupportsBiometrics method call, and replace the code I linked to above with a call to that.
(I'd like to do it for iOS at some point, but for this PR we should just do Android).
Additionally, we could add a second method to the interface (e.g.
getPresentBiometrics) that just returns a list of all present biometric types, disregarding wether they are enrolled or not, so that this functionality remains available on Android. (But this would then need to also be implemented on iOS)
I'm not sure what you mean by "this functionality"; the functionality that we need to preserve is just deviceSupportsBiometrics working.
There was a problem hiding this comment.
It's not ignored, it's how this works.
Am I missing something? here the value seems to be ignored (same on iOS), which results in an empty list regardless of whether the undefined value was returned by the native side, which is what deviceSupportsBiometrics uses there.
the functionality that we need to preserve is just deviceSupportsBiometrics working.
Right, which needs to return true even if those biometrics are not enrolled, which would then mean deviceSupportsBiometrics is broken on iOS as well, as undefined is ignored there too.
conclusion being that deviceSupportsBiometrics needs its own method channel call on both platforms, correct?
There was a problem hiding this comment.
Am I missing something? here the value seems to be ignored (same on iOS)
🤦🏻
Even after all the discussion about this when you initially extracted the default method channel implementation, I still missed that in review. That's a regression from the refactor: deviceSupportsBiometrics is not supposed to be calling the Dart function, it's supposed to be directly invoking the method channel call:
dd2cd96#diff-41eeb8757de140d2fbb1c3650b056c1721926ef417103bf2084a3cc48d1f504fR126-R129
Right, which needs to return true even if those biometrics are not enrolled, which would then mean deviceSupportsBiometrics is broken on iOS as well
Yes, the regression broke iOS too. And also any third-party implementations that relied on this behavior (likely that's none, but it's still wrong). We need to fix this in all three packages.
For the default method channel we need to restore calling the method channel directly. For iOS and Android, we should add a new method. If we didn't already know how confusing the undefined implementation was, the fact that I was specifically watching for it in the refactor and still missed that the final version wasn't right certainly confirms it.
There was a problem hiding this comment.
I can put together a PR for the default method channel now, and this PR can be updated to fix Android. Can you do a PR after this one to mirror what you add here to the iOS implementation to fix that regression?
There was a problem hiding this comment.
Alright, can do. I'll fix this for Android now, and I'll try submit a fix for the iOS implementation tomorrow.
There was a problem hiding this comment.
...h/local_auth_android/android/src/main/java/io/flutter/plugins/localauth/LocalAuthPlugin.java
Show resolved
Hide resolved
…resent but not enrolled.
Fixes a regression from the federation of local_auth in the default method channel implementation of `deviceSupportsBiometrics`. It needs to use the unfiltered method channel list in order to work correctly, not the results of the Dart method that filterns out the sentinel for unenrolled devices. See flutter#5309 (comment) for context.
Fixes a regression from the federation of local_auth in the default method channel implementation of `deviceSupportsBiometrics`. It needs to use the unfiltered method channel list in order to work correctly, not the results of the Dart method that filterns out the sentinel for unenrolled devices. See flutter#5309 (comment) for context.
...h/local_auth_android/android/src/main/java/io/flutter/plugins/localauth/LocalAuthPlugin.java
Outdated
Show resolved
Hide resolved
...uth/local_auth_android/android/src/test/java/io/flutter/plugins/localauth/LocalAuthTest.java
Show resolved
Hide resolved
BeMacized
added
the
waiting for tree to go green
…ed biometrics on Android. (flutter/plugins#5309)
|
Shouldn't the README be changed? |
|
@RafaRuiz This PR has already been submitted, so cannot be changed; if there are other changes to make please file an issue. |
|
I've sent a PR indeed |
This PR fixes the issue where the Android implementation of getEnrolledBiometrics returned biometrics that have present hardware on the device, but are not enrolled. This brings it in line with the behaviour on iOS.
Relevant issue:
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.