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

[local_auth] Api to stop authentication #2111

Merged
mehmetf merged 9 commits into from
Oct 30, 2019
Merged

[local_auth] Api to stop authentication #2111

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eeaa591
[local_auth] Added api to cancel android fingerprint authentication.
karan-rawal Sep 3, 2019
a8a84f9
[local_auth] Updated readme.
karan-rawal Sep 3, 2019
bf5c75c
Merge branch 'master' into ApiToStopAuthentication
karan-rawal Sep 11, 2019
ca88f26
Fixed Platform.isAndroid issue.
karan-rawal Sep 26, 2019
ce85ba6
Sync with remote
karan-rawal Sep 26, 2019
93fab07
Sync with master
karan-rawal Oct 3, 2019
aed3a2b
Merge branch 'master' into ApiToStopAuthentication
karan-rawal Oct 14, 2019
2ed420b
[local_auth] Fixed auth PR reviews.
karan-rawal Oct 25, 2019
0c31915
Merge branch 'master' into ApiToStopAuthentication
karan-rawal Oct 30, 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
Prev Previous commit
Next Next commit
[local_auth] Fixed auth PR reviews.
  • Loading branch information
@karan-rawal
karan-rawal committed Oct 25, 2019
commit 2ed420b3792f0ef8fef0e40650fb685143be0547
2 changes: 1 addition & 1 deletion packages/local_auth/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## 0.6.0+3
## 0.6.1

* Added ability to stop authentication (For Android).

Expand Down
3 changes: 2 additions & 1 deletion packages/local_auth/android/src/main/java/io/flutter/plugins/localauth/LocalAuthPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private LocalAuthPlugin(Registrar registrar) {
@Override
public void onMethodCall(MethodCall call, final Result result) {
if (call.method.equals("authenticateWithBiometrics")) {
if (!authInProgress.compareAndSet(false, true)) {
if (authInProgress.get()) {
// Apps should not invoke another authentication request while one is in progress,
// so we classify this as an error condition. If we ever find a legitimate use case for
// this, we can try to cancel the ongoing auth and start a new one but for now, not worth
Expand All @@ -60,6 +60,7 @@ public void onMethodCall(MethodCall call, final Result result) {
null);
Copy link
Contributor

@mehmetf mehmetf Oct 24, 2019

Choose a reason for hiding this comment

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

There's a bug here for authInProgress. We might set it to true and never actually call authenticate.

Could you chage line 41 to: if (authInProgress.get())
then add to line 62: authInProgress.set(true);.

Since this method always runs in the UIThread, we technically don't need to use AtomicBoolean. You can also optionally change all of this to a normal boolean primitive.

Copy link
Contributor Author

@karan-rawal karan-rawal Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Resolved. Not changing it to boolean primitive. Since, I don't want things to break. :)

return;
}
authInProgress.set(true);
authenticationHelper =
new AuthenticationHelper(
(FragmentActivity) activity,
Expand Down
5 changes: 2 additions & 3 deletions packages/local_auth/lib/local_auth.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,15 @@ class LocalAuthentication {
}

/// Returns true if auth was cancelled successfully.
/// This api only works for Android.
/// Returns false if there was some error or no auth in progress.
///
/// Returns [Future] bool true or false:
Future<bool> stopAuthentication() {
Copy link
Contributor

@mehmetf mehmetf Oct 24, 2019

Choose a reason for hiding this comment

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

Out of curiosity, how would the Dart app decide to call this?

Copy link
Contributor Author

@karan-rawal karan-rawal Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For devices with in-display fingerprint sensor(assuming that authentication is in progress), if user clicks on any other button on the screen, the fingerprint scanning will still be in progress. Hence, we needed an API to stop the authentication in such scenarios.

Copy link
Contributor

@mehmetf mehmetf Oct 25, 2019

Choose a reason for hiding this comment

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

Thanks for the response!

If user clicks on any other button on the screen

This is the part that I don't get. Why would I allow user to click anything in my app if there's authentication in progress? The user is not authenticated so should not be interacting with the app at all. I was assuming that on-screen auth will show exactly the same dialog. The only difference would be where you touch.

Could you please point me to a video or an article of some sort which shows cancelAuthentication in action?

Copy link
Contributor Author

@karan-rawal karan-rawal Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@mehmetf For in-display fingerprint scanners, we don't get any popup. We just get the fingerprint scanner icon.

http://tiny.cc/e345ez

Assuming current behaviour of the plugin
In the link, you can see that we only get the fingerprint scanner icon and no popup. The user can still type the passcode, and click done. When user clicks done, and we navigate to another screen the authentication is still going on. The plugin doesn't provide any mechanism to cancel this authentication.

So this PR should solve the above problem.

I hope I was clear in explaining. :)

Copy link
Contributor

@mehmetf mehmetf Oct 25, 2019

Choose a reason for hiding this comment

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

Very interesting.

Something is broken in this flow though.

  • The image shows FP and a PIN and user can choose to authenticate either way.
  • If both of these are not appearing due to this plugin then who is displaying the PIN entry method? That seems wrong. So, I assume both of these are appearing due to the plugin calling biometric.authenticate.
  • So, assuming biometric API is showing the PIN entry, if user enters PIN instead of FP, why would biometric.authenticate not return "success"? Why would we still expect a fingerprint scan?

I am OK adding this API because it is supported on Android. However the use case you are describing seems very very wrong to me. The app should not be responsible for cancelling authentication if the user already authenticated.

If you agree with me, could you take some time to research how the Android API is supposed to be used? In particular why would PIN entry not run success/failure callbacks? (https://github.com/flutter/plugins/blob/master/packages/local_auth/android/src/main/java/io/flutter/plugins/localauth/AuthenticationHelper.java#L142)

Copy link
Contributor Author

@karan-rawal karan-rawal Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@mehmetf Thanks for your quick response.

  1. The screenshot is not from my app but explains the exact scenario we need in our app. The screenshot is from oneplus's app locker screen.
  2. The PIN entry is not due to the plugin. It's the app's screen having PIN entry, and biometric.authenticate just overlays the fingerprint icon on top of it for scanning (The user can still interact with other part of the screen though).
  3. Because the biometric API is not responsible for showing the PIN entry, the biometric.authenticate doesn't know anything about it. There's no relation between them. :)

I'm not sure if the use case is wrong, but I have seen this flow in a lot of apps.
Just to add, the biometric API does show fingerprint dialog for physical fingerprint sensors. But it just shows fingerprint icon overlay for in-screen fingerprint sensors.

Copy link
Contributor

@mehmetf mehmetf Oct 25, 2019

Choose a reason for hiding this comment

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

I see!

OK that makes more sense. So basically you app is offering two auth methods simultaneously and you want to cancel one if the user chooses the other one.

Copy link
Contributor Author

@karan-rawal karan-rawal Oct 25, 2019

Choose a reason for hiding this comment

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

@mehmetf Exactly. :)

if (_platform.isAndroid) {
return _channel.invokeMethod<bool>('stopAuthentication');
}
final Future<bool> future = Future<bool>(() => true);
Completer<bool>().complete(future);
return future;
return Future<bool>.sync(() => true);
}

/// Returns true if device is capable of checking biometrics
Expand Down
4 changes: 2 additions & 2 deletions 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 <[email protected]>
homepage: https://github.com/flutter/plugins/tree/master/packages/local_auth
version: 0.6.0+3
version: 0.6.1

flutter:
plugin:
Expand All @@ -16,7 +16,7 @@ dependencies:
sdk: flutter
meta: ^1.0.5
intl: ">=0.15.1 <0.17.0"
platform: ^2.0.0
platform: ^2.0.0

dev_dependencies:
flutter_test:
Expand Down