[local_auth] Avoid user confirmation on face unlock #2047
Conversation
mehmetf
commented
Sep 7, 2019
mehmetf
commented
- 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 (fixes the TODO in code).
|
@mklim Let me know if you have any comments. |
| @@ -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" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -77,6 +77,7 @@ public AuthenticationHelper( | |||
| .setTitle((String) call.argument("signInTitle")) | |||
| .setSubtitle((String) call.argument("fingerprintHint")) | |||
| .setNegativeButtonText((String) call.argument("cancelButton")) | |||
| .setConfirmationRequired(false) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
- I want Android to match iOS' behavior and never show a confirmation dialog for face unlock.
- I want to add some extra confirmation to sensitive face unlock transactions, but only if they execute just on Android.
- 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.
- 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.
- Seems like it risks introducing security issues on Android specifically since the best practice on that platform requires extra behavior that iOS does not.
- This seems like an unlikely hypothetical use case.
- 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.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Code changes LG, thanks for iterating on this!
Could you also add a test? We're still being largely limited by not being able to directly test platform code, but we can at least check that sensitiveTransaction is getting passed through and defaulting to true on the Dart API.
|
I added a test but that also meant switching from dart:io to package:platform so we can mock the platform. |
mklim
added
submit queue
* Define a new parameter for signaling that the transaction is sensitive. * Up the biometric version to beta01. * Handle no device credential error.
* Define a new parameter for signaling that the transaction is sensitive. * Up the biometric version to beta01. * Handle no device credential error.
* Define a new parameter for signaling that the transaction is sensitive. * Up the biometric version to beta01. * Handle no device credential error.
