[local_auth] Fix failed biometric authentication not throwing error #6821

LouiseHsu · 2022-12-10T02:25:40Z

On iPhones which support TouchID, when failing biometric authentication multiple times, no error will be thrown. This is because the error code that is being thrown is not being handled - this PR fixes that issues, and also ensures that all error codes regardless if known will be handled and thus throw an appropriate error. This also touches old tests, which were using dummy errors codes - they have now been replaced with appropriate errors for the test.

List which issues are fixed by this PR. You must list at least one issue.

Fixes flutter/flutter#103380

If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].

Pre-launch Checklist

This reverts commit 89ba69c.

This reverts commit 684790a.

jmagman · 2022-12-12T20:15:36Z

Removing myself until this out of draft and ready to review 🙂

jmagman · 2022-12-12T20:18:36Z

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#103380

Change this to "fixes: flutter/flutter#103380" to link the issue for auto-close
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

hellohuanlin · 2022-12-12T20:09:35Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

@@ -41,6 +41,10 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result
    [self deviceSupportsBiometrics:result];
  } else if ([@"isDeviceSupported" isEqualToString:call.method]) {
    result(@YES);
+  } else if ([@"handleAuthReplyWithSuccess" isEqualToString:call.method]) {
+    bool success = [call.arguments[@"success"] boolValue];


hellohuanlin · 2022-12-12T20:10:24Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

@@ -41,6 +41,10 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result
    [self deviceSupportsBiometrics:result];
  } else if ([@"isDeviceSupported" isEqualToString:call.method]) {
    result(@YES);
+  } else if ([@"handleAuthReplyWithSuccess" isEqualToString:call.method]) {
+    bool success = [call.arguments[@"success"] boolValue];
+    NSError* error = call.arguments[@"error"];


is call.arguments[@"error"] indeed NSError type?

It should be!

plugins/packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

Line 215 in abb8e68

error:(NSError *)error

can you double check by setting a break point here and inspecting the type of error?

hellohuanlin · 2022-12-12T20:12:13Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-      // TODO(stuartmorgan): Remove the pragma and s/TouchID/Biometry/ in these constants when
-      // iOS 10 support is dropped. The values are the same, only the names have changed.


is ios 10 dropped?

Yes (?), I was under the impression minimum ios version is 11.0 now

hmmm, i thought it was ios 9 but i could remember it wrongly. Can you double check other plugins?

On the flutter docs it says minimum version is 11 https://docs.flutter.dev/deployment/ios#:~:text=The%20minimum%20iOS%20version%20that,to%20the%20highest%20required%20version.

that's the current flutter version (people could be using older flutter)

quick search and found this - we are still at ios 9 here.

Huan is right, this version of the plugin should continue to work on iOS 9 until the minimum version is bumped to 11. The tests are passing because PRs only run on Flutter master, which is auto-migrating the example app to iOS 11 so there's no issue. I think that would even happen in stable too, which only gets run on post-submit.

Trying to think how to force the app to run against the iOS 9 SDK in CI. I guess we just haven't run into issues related to this yet... cc @stuartmorgan

Trying to think how to force the app to run against the iOS 9 SDK in CI.

Yeah, that's a problem given the auto-migrate logic. I would suggest someone just do a mass-update of all the iOS implementation packages to Flutter 3.3 and iOS 11 now (or maybe 3.3 first, and then iOS 11 as a second pass if that's non-trivial due to dead code removal).

That's the approach I've adopted for our old-Flutter-version support: when we update the N-2 stable version in CI, I mass-update all plugins to at least that minimum version, because we're no longer getting any automated testing that we aren't accidentally breaking people using older versions. (That kind of update doesn't cause any churn for users, because people still using old Flutter versions will just not be given the updates they can't use due to the resolver constraint on the Flutter version.)

hellohuanlin · 2022-12-12T20:17:42Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

+      case LAErrorSystemCancel:
+        if ([arguments[@"stickyAuth"] boolValue]) {
+            self->_lastCallArgs = arguments;
+            self->_lastResult = result;


Do we need result(@YES) here?

I think result(@yES) should only get returned if it's successful, so if the system cancels it should return no error and exit silently

can you explain what is stickyAuth is?

I believe the result must be called with a value or with an error, so that the dart side does not keep waiting (tho this is probably not related to your PR).

from what I can tell, stickyauth makes it so that when you leave then return to the app, the authentication screen persists. I didn't add it btw, I just moved the cases around

As for the result, that makes sense - should it return result(@no) instead? Since otherwise it would imply the authentication was successful

I looked into the doc. It's intended behavior that we don't call the result callback here. So we can just leave it as it is.

hellohuanlin · 2022-12-12T20:20:31Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

      case LAErrorUserFallback:
+      default:


when will this default be called?

So I added the default so it would catch any unknown error codes and throw an error - the original issue was happening because an error code was thrown that wasnt listed and it was failing silently. Should I change my approach?

the original issue was happening because an error code was thrown that wasnt listed and it was failing silently.

can you inspect the error code and explicitly list here? The default case shadows all potential future cases and may cause hard-to-find bugs.

the error code was

plugins/packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

Line 232 in abb8e68

case LAErrorAuthenticationFailed:

So I already added it to the list
Should I remove the default case then?

I agree, we should handle all enums and remove the default to detect (via compilation warning) when this problem happens again the next time we update the SDK.

jmagman · 2022-12-12T21:42:46Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

+  [plugin handleMethodCall:call
+                    result:^(id _Nullable result) {
+                      XCTAssertTrue([NSThread isMainThread]);
+                      XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);


Suggested change

XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);

XCTAssertTrue([result isKindOfClass:[FlutterError class]]);

jmagman · 2022-12-12T21:46:47Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

@@ -41,6 +41,10 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result
    [self deviceSupportsBiometrics:result];
  } else if ([@"isDeviceSupported" isEqualToString:call.method]) {
    result(@YES);
+  } else if ([@"handleAuthReplyWithSuccess" isEqualToString:call.method]) {


Hm, why was this added? These should correspond to the messages coming from the dart API, like:

plugins/packages/local_auth/local_auth_platform_interface/lib/default_method_channel_platform.dart

Lines 78 to 79 in 2eb6165

Future<bool> isDeviceSupported() async =>

(await _channel.invokeMethod<bool>('isDeviceSupported')) ?? false;

Oh I added it because thats how the other tests were testing methods - Ill test it another way then, good to know its just for dart API messages

jmagman · 2022-12-12T21:48:02Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

      case LAErrorUserFallback:
+      default:


I agree, we should handle all enums and remove the default to detect (via compilation warning) when this problem happens again the next time we update the SDK.

jmagman · 2022-12-12T21:53:10Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-      // TODO(stuartmorgan): Remove the pragma and s/TouchID/Biometry/ in these constants when
-      // iOS 10 support is dropped. The values are the same, only the names have changed.


Huan is right, this version of the plugin should continue to work on iOS 9 until the minimum version is bumped to 11. The tests are passing because PRs only run on Flutter master, which is auto-migrating the example app to iOS 11 so there's no issue. I think that would even happen in stable too, which only gets run on post-submit.

Trying to think how to force the app to run against the iOS 9 SDK in CI. I guess we just haven't run into issues related to this yet... cc @stuartmorgan

jmagman · 2022-12-12T21:53:56Z

Address the format issues:
https://cirrus-ci.com/task/6643342537129984?logs=format#L19

stuartmorgan · 2022-12-13T05:11:30Z

packages/local_auth/local_auth/pubspec.yaml

@@ -3,7 +3,7 @@ description: Flutter plugin for Android and iOS devices to allow local
  authentication via fingerprint, touch ID, face ID, passcode, pin, or pattern.
 repository: https://github.com/flutter/plugins/tree/main/packages/local_auth/local_auth
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+local_auth%22
-version: 2.1.2
+version: 2.1.3


No updates to local_auth are needed in this case, just the implementation package.

stuartmorgan · 2022-12-13T05:12:32Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

@@ -27,451 +27,543 @@ @interface FLTLocalAuthPluginTests : XCTestCase
 @implementation FLTLocalAuthPluginTests

 - (void)setUp {
-  self.continueAfterFailure = NO;
+    self.continueAfterFailure = NO;


You'll probably want to set your Xcode settings (or whatever editor you are using) to 2-space indent for ObjC :)

stuartmorgan · 2022-12-13T12:09:11Z

FYI if you install clang-format (go/clang-format-mac has pointers internally) you can just run the format step locally to get the format that CI is expecting.

jmagman · 2022-12-13T23:46:32Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-      // TODO(stuartmorgan): Remove the pragma and s/TouchID/Biometry/ in this constant when
-      // iOS 10 support is dropped. The values are the same, only the names have changed.
-    case LAErrorTouchIDNotEnrolled:


Does this need to be put back too then?

Yes, missed that thanks!

jmagman · 2022-12-14T01:33:23Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

+  [plugin handleMethodCall:call
+                    result:^(id _Nullable result) {
+                      XCTAssertTrue([NSThread isMainThread]);
+                      XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);


jmagman · 2022-12-14T01:34:54Z

packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m

@@ -216,26 +216,28 @@ - (void)handleAuthReplyWithSuccess:(BOOL)success
    result(@YES);
  } else {
    switch (error.code) {
-      case LAErrorPasscodeNotSet:
+      case LAErrorSystemCancel:


You could put this back to where it was at the bottom to avoid dirtying the git history and just add the result(@NO) part.

jmagman · 2022-12-14T01:36:40Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

                      XCTAssertTrue([NSThread isMainThread]);
-                      XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
-                      XCTAssertFalse([result boolValue]);


Did you mean to change these result blocks?

jmagman · 2022-12-14T01:36:45Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

-                      XCTAssertTrue([NSThread isMainThread]);
-                      XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
                      OCMVerify([mockAuthContext setLocalizedFallbackTitle:localizedFallbackTitle]);
-                      XCTAssertFalse([result boolValue]);


jmagman · 2022-12-14T01:36:54Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

-                      XCTAssertTrue([NSThread isMainThread]);
-                      XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
                      OCMVerify([mockAuthContext setLocalizedFallbackTitle:nil]);
-                      XCTAssertFalse([result boolValue]);


So I changed it because in these tests, they were originally using a random errorCode of 99 which is not an enum value, so it was falling through the switch case and returning result(No) instead of throwing an error. I changed them to use appropriate error codes instead cuz I thought it would be more accurate to throw an error, but what do you think? Should I just leave it as it was before?

For the second and third copy of this, the fact that it's simulating an error and asserting things about the error is a copypasta issue in these tests that I missed in review; there's no reason for a test about localized fallback title handling to be using the error path, or asserting anything other than the title part and the main thread. They were just created by copying and pasting from the closest test in the file it looks like. We should just update the dummy reply to a success path, and remove the irrelevant assertions.

For the first, if we intentionally have a codepath that returns NO instead of throwing an error, we should ideally test both that and the error path. The random error code test simulates what would happen if Apple added a new error that we didn't handle yet, for instance.

The random error code test simulates what would happen if Apple added a new error that we didn't handle yet, for instance.

+1, there should be a test for a new enum that we haven't handled yet.

jmagman · 2022-12-15T18:58:30Z

packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

+  [self waitForExpectationsWithTimeout:kTimeout handler:nil];
+}
+
+- (void)testFailedWithKnownErrorCode {


I think testFailedAuthWithBiometrics already handles a "known error code", this is more testing LAErrorTouchIDNotEnrolled explicitly, right?

Suggested change

- (void)testFailedWithKnownErrorCode {

- (void)testFailedTouchIDNotEnrolled {

Youre right, it's kind of a dupe - Ill remove the test

hellohuanlin · 2022-12-15T22:23:46Z

I saw multiple email updates. Let me know when it's ready for a final review.

jmagman

LGTM!

You should update the description with Fixes https://github.com/flutter/flutter/issues/103380 so GitHub will auto-link the issue to the PR and close the issue for you. I manually linked them this time.
When you're ready to merge, put the autosubmit label on the issue and the bot will do the rest, and the plugin will be published.

…ng error (flutter/plugins#6821)

* 5f62d21eb [local_auth] Fix failed biometric authentication not throwing error (flutter/plugins#6821) * ca974ab0c [webview_flutter_web] Copies web implementation of webview_flutter from v4_webview (flutter/plugins#6854) * 4d11be416 [image_picker] Don't store null paths in lost cache (flutter/plugins#6678) * fd2841fd0 [webview_flutter_android] Fix timeouts in the integration tests (flutter/plugins#6857) * abc9f9a9b [flutter_plugin_tools] If `clang-format` does not run, fall back to other executables in PATH (flutter/plugins#6853) * 7efb5e89d [video_player] Add compatibility with the current platform interface (flutter/plugins#6855) * 32dcbf3e3 [image_picker] Improve image_picker for iOS to handle more image types (flutter/plugins#6812) * 840a04954 [webview_flutter] Copies app-facing implementation of webview_flutter from v4_webview (flutter/plugins#6856)

…#117314) * 5f62d21eb [local_auth] Fix failed biometric authentication not throwing error (flutter/plugins#6821) * ca974ab0c [webview_flutter_web] Copies web implementation of webview_flutter from v4_webview (flutter/plugins#6854) * 4d11be416 [image_picker] Don't store null paths in lost cache (flutter/plugins#6678) * fd2841fd0 [webview_flutter_android] Fix timeouts in the integration tests (flutter/plugins#6857) * abc9f9a9b [flutter_plugin_tools] If `clang-format` does not run, fall back to other executables in PATH (flutter/plugins#6853) * 7efb5e89d [video_player] Add compatibility with the current platform interface (flutter/plugins#6855) * 32dcbf3e3 [image_picker] Improve image_picker for iOS to handle more image types (flutter/plugins#6812) * 840a04954 [webview_flutter] Copies app-facing implementation of webview_flutter from v4_webview (flutter/plugins#6856)

…lutter#6821) * fix failed biometric authentication not throwing error + tests * fix test names * Revert "fix test names" This reverts commit 89ba69c. * Revert "fix failed biometric authentication not throwing error + tests" This reverts commit 684790a. * fix authentication not throwing error + tests * fix test name * auto format * cr fixes * addressed pr comments * formatting * formatting * formatting * format attempt * format attempt * formatting fixes * change incorrect versionning * fix test * add back macro * fixed up tests, removed unnecessary assertions, replaced isAMemberOf * add back error for unknown error codes * tests * changed enum to something thats not deprecated * remove redundant test

LouiseHsu added 7 commits December 9, 2022 17:32

fix failed biometric authentication not throwing error + tests

684790a

fix test names

89ba69c

Revert "fix test names"

823843d

This reverts commit 89ba69c.

Revert "fix failed biometric authentication not throwing error + tests"

3da994d

This reverts commit 684790a.

fix authentication not throwing error + tests

714907d

fix test name

abb8e68

auto format

b7bbcab

LouiseHsu requested a review from hellohuanlin as a code owner December 10, 2022 02:25

github-actions bot added p: local_auth platform-ios labels Dec 10, 2022

LouiseHsu requested a review from jmagman December 10, 2022 02:26

LouiseHsu marked this pull request as draft December 10, 2022 02:26

jmagman removed their request for review December 12, 2022 20:15

hellohuanlin reviewed Dec 12, 2022

View reviewed changes

jmagman reviewed Dec 12, 2022

View reviewed changes

cr fixes

cc7e32f

LouiseHsu marked this pull request as ready for review December 12, 2022 22:07

LouiseHsu requested a review from stuartmorgan as a code owner December 12, 2022 22:07

stuartmorgan reviewed Dec 13, 2022

View reviewed changes

LouiseHsu added 5 commits December 13, 2022 02:34

addressed pr comments

0bcffeb

formatting

5a545bd

formatting

1a053b2

formatting

2a64876

format attempt

2583cc4

format attempt

40a00cf

LouiseHsu added 2 commits December 13, 2022 14:06

formatting fixes

0ac9aee

change incorrect versionning

699a612

jmagman reviewed Dec 13, 2022

View reviewed changes

LouiseHsu added 2 commits December 13, 2022 15:51

fix test

b483520

add back macro

737bbd1

jmagman reviewed Dec 14, 2022

View reviewed changes

LouiseHsu added 4 commits December 14, 2022 15:50

fixed up tests, removed unnecessary assertions, replaced isAMemberOf

f62195a

add back error for unknown error codes

56d4969

tests

99e34ca

changed enum to something thats not deprecated

c70c8bc

jmagman reviewed Dec 15, 2022

View reviewed changes

remove redundant test

42682f2

jmagman changed the title ~~[local-auth] Fix failed biometric authentication not throwing error~~ [local_auth] Fix failed biometric authentication not throwing error Dec 15, 2022

jmagman linked an issue Dec 16, 2022 that may be closed by this pull request

[local_auth] IOS will not return any error if locked by the system flutter/flutter#103380

Closed

jmagman approved these changes Dec 16, 2022

View reviewed changes

LouiseHsu added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 16, 2022

auto-submit bot merged commit 5f62d21 into flutter:main Dec 16, 2022

engine-flutter-autoroll mentioned this pull request Dec 19, 2022

Roll Plugins from cbcf50726fb9 to 840a04954fa0 (8 revisions) flutter/flutter#117314

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 19, 2022

5f62d21eb [local_auth] Fix failed biometric authentication not throwi…

3c3b058

…ng error (flutter/plugins#6821)

jmagman mentioned this pull request Dec 28, 2022

[local_auth][iOS]: NotAvailable error code is thrown when user taps on "Don't allow" while trying to allow FaceId recognition. flutter/flutter#102734

Closed

	XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
	XCTAssertTrue([result isKindOfClass:[FlutterError class]]);

	Future<bool> isDeviceSupported() async =>
	(await _channel.invokeMethod<bool>('isDeviceSupported')) ?? false;

	- (void)testFailedWithKnownErrorCode {
	- (void)testFailedTouchIDNotEnrolled {

[local_auth] Fix failed biometric authentication not throwing error #6821

[local_auth] Fix failed biometric authentication not throwing error #6821

Conversation

LouiseHsu commented Dec 10, 2022 • edited Loading

Pre-launch Checklist

jmagman commented Dec 12, 2022

jmagman commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

stuartmorgan Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LouiseHsu Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LouiseHsu Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

jmagman commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LouiseHsu Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin commented Dec 15, 2022

jmagman left a comment

Choose a reason for hiding this comment

LouiseHsu commented Dec 10, 2022 •

edited

Loading

jmagman Dec 12, 2022 •

edited

Loading

stuartmorgan Dec 13, 2022 •

edited

Loading

LouiseHsu Dec 12, 2022 •

edited

Loading

LouiseHsu Dec 12, 2022 •

edited

Loading

jmagman Dec 12, 2022 •

edited

Loading

LouiseHsu Dec 14, 2022 •

edited

Loading