Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[local_auth] Avoid user confirmation on face unlock #2047

Merged
mklim merged 3 commits into from
Sep 10, 2019
Merged

[local_auth] Avoid user confirmation on face unlock #2047

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4898c5
[local_auth] Avoid user confirmation on face unlock
mehmetf Sep 7, 2019
6a0d4e9
Fix review comments
mehmetf Sep 10, 2019
ea55046
Add a test. Use a mockable platform
mehmetf Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/local_auth/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 0.5.4

* Do not require confirmation for face unlock on Android bringing the behavior in line with iOS..
* Up the biometric version to beta01.
* Handle no device credential error.

## 0.5.3

* Add face id detection as well by not relying on FingerprintCompat.
Expand Down
2 changes: 1 addition & 1 deletion packages/local_auth/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ android {

dependencies {
api "androidx.core:core:1.1.0-beta01"
api "androidx.biometric:biometric:1.0.0-alpha04"
api "androidx.biometric:biometric:1.0.0-beta01"
Copy link
Contributor

@mklim mklim Sep 9, 2019

Choose a reason for hiding this comment

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

In general we've been trying to guard dependency upgrades behind breaking version change bumps, out of an abundance of caution for when there's breaking changes in the dependency itself.

Copy link
Contributor Author

@mehmetf mehmetf Sep 9, 2019

Choose a reason for hiding this comment

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

Both androidx.biometrics and this plugin is not 1.0 yet. So breaking changes are expected. Having said that, I am happy to bump version to 0.6. Would that be sufficient?

Copy link
Contributor

@mklim mklim Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

0.6 SGTM, thanks. The pub convention for semantic versioning plugins under 1.0.0 is 0.major.minor+patch instead of major.minor.patch, so 0.6.0 would be the breaking change update here.

https://dart.dev/tools/pub/versioning#semantic-versions

api "androidx.fragment:fragment:1.1.0-alpha06"
}
13 changes: 6 additions & 7 deletions ...s/local_auth/android/src/main/java/io/flutter/plugins/localauth/AuthenticationHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public AuthenticationHelper(
.setTitle((String) call.argument("signInTitle"))
.setSubtitle((String) call.argument("fingerprintHint"))
.setNegativeButtonText((String) call.argument("cancelButton"))
.setConfirmationRequired(false)
Copy link
Contributor

@mklim mklim Sep 9, 2019

Choose a reason for hiding this comment

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

Does it make sense to universally implicitly set this to false? I get that this is the default behavior for iOS, but the API docs for Android suggest that this should still be required for higher risk transactions, which I think is out of scope of the plugin itself to know.

Copy link
Contributor Author

@mehmetf mehmetf Sep 9, 2019

Choose a reason for hiding this comment

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

I was thinking about that and it seems to me that you would need this control only if you have a sensitive transaction and you want to execute it only on Android (as iOS does not provide a way to require confirmation afaik). I think that is a far fetched scenario, so I opted to keep the API simple rather than crowding it with an Android specific toggle.

Copy link
Contributor

@mklim mklim Sep 9, 2019

Choose a reason for hiding this comment

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

I think it's a little broader than that, depending on the intention of setConfirmationRequired.
The reason I'm thinking it's a best practice on Android to set this when it applies is that it defaults to true and the docs say to use it for higher risk transactions.

Assuming it's best practice to set it to true whenever the transaction is sensitive, the use case here is for any plugin that runs on Android, has sensitive transactions, and wants to follow the best practice on that platform for using the underlying API. Unfortunately the iOS best practice here is different so making a good unified API isn't totally straightforward. Maybe something like isSensitiveTransaction that gets passed here, defaults to true to match Android's behavior, and is just a non-op on iOS since there isn't an equivalent behavior? I could see a couple different ways to approach it.

If I'm reading too much into it and it's more of just an additional option a developer could set if they personally wanted the extra security dialog, then it's a much narrower use case like you're saying and maybe not worth exposing.

Copy link
Contributor Author

@mehmetf mehmetf Sep 9, 2019

Choose a reason for hiding this comment

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

That's interesting, Michael.

If I were to build a plugin for biometrics that would handle sensitive transactions, I might make a decision to not allow using faceid on iOS at all since it is prone to false positives and unintentional unlocks. We could expose that functionality in this plugin. That would be neat. However, then the question becomes what's the default value of this knob? Android says it must be set to true. If we do that, then we disallow face id by default.

One way to solve this would be to have the isSensitiveTransaction knob show a Flutter dialog and turn off the confirmation dialog on Android native to off by default. The only problem here is that we don't know ahead of time whether the user has fingerprint auth by default or face id. Not sure if there's a way to get that.

WDYT?

Copy link
Contributor

@mklim mklim Sep 10, 2019

Choose a reason for hiding this comment

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

Hmm. Sounds like there's actually many potential situations going on here. :) So far it seems like we've thought of 4 potential use cases.

  1. I want Android to match iOS' behavior and never show a confirmation dialog for face unlock.
  2. I want to add some extra confirmation to sensitive face unlock transactions, but only if they execute just on Android.
  3. I want to follow the best practice of each underlying platform, even though they differ from each other. This means confirmation dialogs on sensitive face unlock transactions on Android but no confirmation dialogs ever when on iOS.
  4. I want to enforce specific security practices myself and never allow face unlock for sensitive transactions without a confirmation dialog, at the cost of not supporting face unlock at all on iOS.

The plugin is currently closest to 3 but not doing it particularly well since it always treats every transaction as sensitive. This PR was originally written for 1.

Written out like this, I think the right direction for the plugin overall is 3.

  1. Seems like it risks introducing security issues on Android specifically since the best practice on that platform requires extra behavior that iOS does not.
  2. This seems like an unlikely hypothetical use case.
  3. Results in inconsistent behavior across platforms, but that's not necessarily bad. The "inconsistent" behavior in terms of how the app specifically behaves is ultimately consistent with the underlying platform's expectations.
  4. I think this is relying on a few assumptions I'm not sure of. It's assuming that a confirmation dialog in and of itself is a valuable security practice that is absolutely needed for sensitive transactions on all platforms that have face unlock. It's deriving this knowledge from Android recommending it as a best practice and coming to the conclusion that therefore iOS face unlock can't be used for sensitive transactions at all. I can think of alternative plausible explanations for why Android recommends it and iOS doesn't that don't lead to dropping face unlock on iOS entirely.

WDYT? Feel free to ping me.

Copy link
Contributor Author

@mehmetf mehmetf Sep 10, 2019

Choose a reason for hiding this comment

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

That's fair. Although I find it a bit cringe-worthy to call a security scenario platform-specific, I agree with your overall assessment. Let's trust iOS to handle this well and provide the knob as a noop on that platform.

.build();
}

Expand All @@ -95,13 +96,11 @@ private void stop() {
@Override
public void onAuthenticationError(int errorCode, CharSequence errString) {
switch (errorCode) {
// TODO(mehmetf): Re-enable when biometric alpha05 is released.
// https://developer.android.com/jetpack/androidx/releases/biometric
// case BiometricPrompt.ERROR_NO_DEVICE_CREDENTIAL:
// completionHandler.onError(
// "PasscodeNotSet",
// "Phone not secured by PIN, pattern or password, or SIM is currently locked.");
// break;
case BiometricPrompt.ERROR_NO_DEVICE_CREDENTIAL:
completionHandler.onError(
"PasscodeNotSet",
"Phone not secured by PIN, pattern or password, or SIM is currently locked.");
break;
case BiometricPrompt.ERROR_NO_SPACE:
case BiometricPrompt.ERROR_NO_BIOMETRICS:
if (call.argument("useErrorDialogs")) {
Expand Down
2 changes: 1 addition & 1 deletion packages/local_auth/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: Flutter plugin for Android and iOS device authentication sensors
such as Fingerprint Reader and Touch ID.
author: Flutter Team <flutter-dev@googlegroups.com>
homepage: https://github.com/flutter/plugins/tree/master/packages/local_auth
version: 0.5.3
version: 0.5.4

flutter:
plugin:
Expand Down