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

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

Merged
merged 23 commits into from
Dec 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
684790a
fix failed biometric authentication not throwing error + tests
LouiseHsu Dec 10, 2022
89ba69c
fix test names
LouiseHsu Dec 10, 2022
823843d
Revert "fix test names"
LouiseHsu Dec 10, 2022
3da994d
Revert "fix failed biometric authentication not throwing error + tests"
LouiseHsu Dec 10, 2022
714907d
fix authentication not throwing error + tests
LouiseHsu Dec 10, 2022
abb8e68
fix test name
LouiseHsu Dec 10, 2022
b7bbcab
auto format
LouiseHsu Dec 10, 2022
cc7e32f
cr fixes
LouiseHsu Dec 12, 2022
0bcffeb
addressed pr comments
LouiseHsu Dec 13, 2022
5a545bd
formatting
LouiseHsu Dec 13, 2022
1a053b2
formatting
LouiseHsu Dec 13, 2022
2a64876
formatting
LouiseHsu Dec 13, 2022
2583cc4
format attempt
LouiseHsu Dec 13, 2022
40a00cf
format attempt
LouiseHsu Dec 13, 2022
0ac9aee
formatting fixes
LouiseHsu Dec 13, 2022
699a612
change incorrect versionning
LouiseHsu Dec 13, 2022
b483520
fix test
LouiseHsu Dec 13, 2022
737bbd1
add back macro
LouiseHsu Dec 14, 2022
f62195a
fixed up tests, removed unnecessary assertions, replaced isAMemberOf
LouiseHsu Dec 14, 2022
56d4969
add back error for unknown error codes
LouiseHsu Dec 15, 2022
99e34ca
tests
LouiseHsu Dec 15, 2022
c70c8bc
changed enum to something thats not deprecated
LouiseHsu Dec 15, 2022
42682f2
remove redundant test
LouiseHsu Dec 15, 2022
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
118 changes: 105 additions & 13 deletions packages/local_auth/local_auth_ios/example/ios/RunnerTests/FLTLocalAuthPluginTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ - (void)testFailedAuthWithBiometrics {
void (^reply)(BOOL, NSError *);
[invocation getArgument:&reply atIndex:4];
dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
reply(NO, [NSError errorWithDomain:@"error" code:99 userInfo:nil]);
reply(NO, [NSError errorWithDomain:@"error" code:LAErrorAuthenticationFailed userInfo:nil]);
});
};
OCMStub([mockAuthContext evaluatePolicy:policy localizedReason:reason reply:[OCMArg any]])
Expand All @@ -136,17 +136,112 @@ - (void)testFailedAuthWithBiometrics {
@"localizedReason" : reason,
}];

XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Bump this.

[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
}

- (void)testFailedWithKnownErrorCode {
FLTLocalAuthPlugin *plugin = [[FLTLocalAuthPlugin alloc] init];
id mockAuthContext = OCMClassMock([LAContext class]);
plugin.authContextOverrides = @[ mockAuthContext ];

FlutterMethodCall *call = [FlutterMethodCall methodCallWithMethodName:@"handleAuthReplyWithSuccess"
arguments:@{
@"success" : @NO,
@"error" : [NSError errorWithDomain:@"error"
code:LAErrorPasscodeNotSet
userInfo:nil],
@"stickyAuth" : @NO,
}];

XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
}

- (void)testFailedWithUnknownErrorCode {
FLTLocalAuthPlugin *plugin = [[FLTLocalAuthPlugin alloc] init];
id mockAuthContext = OCMClassMock([LAContext class]);
plugin.authContextOverrides = @[ mockAuthContext ];

FlutterMethodCall *call = [FlutterMethodCall methodCallWithMethodName:@"handleAuthReplyWithSuccess"
arguments:@{
@"success" : @NO,
@"error" : [NSError errorWithDomain:@"error"
code:-9999
userInfo:nil],
@"stickyAuth" : @NO,
}];

XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
}

- (void)testHandleAuthReplyFailedWithSystemCancel {
FLTLocalAuthPlugin *plugin = [[FLTLocalAuthPlugin alloc] init];
id mockAuthContext = OCMClassMock([LAContext class]);
plugin.authContextOverrides = @[ mockAuthContext ];

FlutterMethodCall *call = [FlutterMethodCall methodCallWithMethodName:@"handleAuthReplyWithSuccess"
arguments:@{
@"success" : @NO,
@"error" : [NSError errorWithDomain:@"error"
code:LAErrorSystemCancel
userInfo:nil],
@"stickyAuth" : @NO,
}];

XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
XCTAssertFalse([result boolValue]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
}


- (void)testHandleAuthReplySucceeded {
FLTLocalAuthPlugin *plugin = [[FLTLocalAuthPlugin alloc] init];
id mockAuthContext = OCMClassMock([LAContext class]);
plugin.authContextOverrides = @[ mockAuthContext ];

FlutterMethodCall *call = [FlutterMethodCall methodCallWithMethodName:@"handleAuthReplyWithSuccess"
arguments:@{
@"success" : @YES
}];

XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
XCTAssertFalse([result boolValue]);
XCTAssertTrue([result boolValue]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
}


- (void)testFailedAuthWithoutBiometrics {
FLTLocalAuthPlugin *plugin = [[FLTLocalAuthPlugin alloc] init];
id mockAuthContext = OCMClassMock([LAContext class]);
Expand All @@ -163,7 +258,7 @@ - (void)testFailedAuthWithoutBiometrics {
void (^reply)(BOOL, NSError *);
[invocation getArgument:&reply atIndex:4];
dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
reply(NO, [NSError errorWithDomain:@"error" code:99 userInfo:nil]);
reply(NO, [NSError errorWithDomain:@"error" code:LAErrorAuthenticationFailed userInfo:nil]);
});
};
OCMStub([mockAuthContext evaluatePolicy:policy localizedReason:reason reply:[OCMArg any]])
Expand All @@ -179,8 +274,7 @@ - (void)testFailedAuthWithoutBiometrics {
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
XCTAssertFalse([result boolValue]);
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to change these result blocks?

XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
Expand All @@ -203,7 +297,7 @@ - (void)testLocalizedFallbackTitle {
void (^reply)(BOOL, NSError *);
[invocation getArgument:&reply atIndex:4];
dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
reply(NO, [NSError errorWithDomain:@"error" code:99 userInfo:nil]);
reply(NO, [NSError errorWithDomain:@"error" code:LAErrorUserFallback userInfo:nil]);
});
};
OCMStub([mockAuthContext evaluatePolicy:policy localizedReason:reason reply:[OCMArg any]])
Expand All @@ -220,10 +314,9 @@ - (void)testLocalizedFallbackTitle {
XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
OCMVerify([mockAuthContext setLocalizedFallbackTitle:localizedFallbackTitle]);
XCTAssertFalse([result boolValue]);
Comment on lines -223 to -226
Copy link
Member

Choose a reason for hiding this comment

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

Same.

XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
Expand All @@ -245,7 +338,7 @@ - (void)testSkippedLocalizedFallbackTitle {
void (^reply)(BOOL, NSError *);
[invocation getArgument:&reply atIndex:4];
dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
reply(NO, [NSError errorWithDomain:@"error" code:99 userInfo:nil]);
reply(NO, [NSError errorWithDomain:@"error" code:LAErrorUserFallback userInfo:nil]);
});
};
OCMStub([mockAuthContext evaluatePolicy:policy localizedReason:reason reply:[OCMArg any]])
Expand All @@ -260,10 +353,9 @@ - (void)testSkippedLocalizedFallbackTitle {
XCTestExpectation *expectation = [self expectationWithDescription:@"Result is called"];
[plugin handleMethodCall:call
result:^(id _Nullable result) {
XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isKindOfClass:[NSNumber class]]);
OCMVerify([mockAuthContext setLocalizedFallbackTitle:nil]);
XCTAssertFalse([result boolValue]);
Comment on lines -263 to -266
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

@LouiseHsu LouiseHsu Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

XCTAssertTrue([NSThread isMainThread]);
XCTAssertTrue([result isMemberOfClass:[FlutterError class]]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
Expand Down
47 changes: 20 additions & 27 deletions packages/local_auth/local_auth_ios/ios/Classes/FLTLocalAuthPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Copy link
Member

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

@LouiseHsu LouiseHsu Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

bool success = [call.arguments[@"success"] boolValue];
Copy link
Contributor

Choose a reason for hiding this comment

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

BOOL

NSError* error = call.arguments[@"error"];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be!

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

Line 215 in abb8e68

error:(NSError *)error

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-12-12 at 1 23 04 PM

[self handleAuthReplyWithSuccess:success error:error flutterArguments:call.arguments flutterResult:result];
} else {
result(FlutterMethodNotImplemented);
}
Expand Down Expand Up @@ -216,41 +220,35 @@ - (void)handleAuthReplyWithSuccess:(BOOL)success
result(@YES);
} else {
switch (error.code) {
case LAErrorSystemCancel:
Copy link
Member

Choose a reason for hiding this comment

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

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.

if ([arguments[@"stickyAuth"] boolValue]) {
self->_lastCallArgs = arguments;
self->_lastResult = result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need result(@YES) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else {
result(@NO);
}
return;
case LAErrorPasscodeNotSet:
#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.
Copy link
Contributor

Choose a reason for hiding this comment

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

is ios 10 dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@jmagman jmagman Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

case LAErrorTouchIDNotAvailable:
case LAErrorTouchIDNotEnrolled:
case LAErrorTouchIDLockout:
#pragma clang diagnostic pop
case LAErrorAuthenticationFailed:
case LAErrorBiometryNotAvailable:
case LAErrorBiometryNotEnrolled:
case LAErrorBiometryLockout:
case LAErrorUserFallback:
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

when will this default be called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@LouiseHsu LouiseHsu Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

[self handleErrors:error flutterArguments:arguments withFlutterResult:result];
return;
case LAErrorSystemCancel:
if ([arguments[@"stickyAuth"] boolValue]) {
self->_lastCallArgs = arguments;
self->_lastResult = result;
return;
}
}
result(@NO);
}
}


- (void)handleErrors:(NSError *)authError
flutterArguments:(NSDictionary *)arguments
withFlutterResult:(FlutterResult)result {
NSString *errorCode = @"NotAvailable";
switch (authError.code) {
case LAErrorPasscodeNotSet:
#pragma clang diagnostic push
#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:
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be put back too then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, missed that thanks!

#pragma clang diagnostic pop
case LAErrorBiometryNotEnrolled:
if ([arguments[@"useErrorDialogs"] boolValue]) {
[self alertMessage:arguments[@"goToSettingDescriptionIOS"]
firstButton:arguments[@"okButton"]
Expand All @@ -260,12 +258,7 @@ - (void)handleErrors:(NSError *)authError
}
errorCode = authError.code == LAErrorPasscodeNotSet ? @"PasscodeNotSet" : @"NotEnrolled";
break;
#pragma clang diagnostic push
#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 LAErrorTouchIDLockout:
#pragma clang diagnostic pop
case LAErrorBiometryLockout:
[self alertMessage:arguments[@"lockOut"]
firstButton:arguments[@"okButton"]
flutterResult:result
Expand Down