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 16 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
4 changes: 4 additions & 0 deletions packages/local_auth/local_auth_ios/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.0.11

* Fixes issue where failed authentication was failing silently

## 1.0.10

* Updates imports for `prefer_relative_imports`.
Expand Down
100 changes: 87 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,12 +136,89 @@ - (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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
- (void)testFailedWithKnownErrorCode {
- (void)testFailedTouchIDNotEnrolled {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

const LAPolicy policy = LAPolicyDeviceOwnerAuthentication;
NSString *reason = @"a reason";
OCMStub([mockAuthContext canEvaluatePolicy:policy error:[OCMArg setTo:nil]]).andReturn(YES);

// evaluatePolicy:localizedReason:reply: calls back on an internal queue, which is not
// guaranteed to be on the main thread. Ensure that's handled correctly by calling back on
// a background thread.
void (^backgroundThreadReplyCaller)(NSInvocation *) = ^(NSInvocation *invocation) {
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:LAErrorBiometryNotEnrolled userInfo:nil]);
});
};
OCMStub([mockAuthContext evaluatePolicy:policy localizedReason:reason reply:[OCMArg any]])
.andDo(backgroundThreadReplyCaller);

FlutterMethodCall *call = [FlutterMethodCall methodCallWithMethodName:@"authenticate"
arguments:@{
@"biometricOnly" : @(NO),
@"localizedReason" : reason,
}];

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

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

const LAPolicy policy = LAPolicyDeviceOwnerAuthentication;
NSString *reason = @"a reason";
OCMStub([mockAuthContext canEvaluatePolicy:policy error:[OCMArg setTo:nil]]).andReturn(YES);

// evaluatePolicy:localizedReason:reply: calls back on an internal queue, which is not
// guaranteed to be on the main thread. Ensure that's handled correctly by calling back on
// a background thread.
void (^backgroundThreadReplyCaller)(NSInvocation *) = ^(NSInvocation *invocation) {
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:LAErrorSystemCancel userInfo:nil]);
});
};
OCMStub([mockAuthContext evaluatePolicy:policy localizedReason:reason reply:[OCMArg any]])
.andDo(backgroundThreadReplyCaller);

FlutterMethodCall *call = [FlutterMethodCall methodCallWithMethodName:@"authenticate"
arguments:@{
@"biometricOnly" : @(NO),
@"localizedReason" : reason,
@"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]);
XCTAssertTrue([result boolValue]);
[expectation fulfill];
}];
[self waitForExpectationsWithTimeout:kTimeout handler:nil];
Expand All @@ -163,7 +240,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 +256,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 +279,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 +296,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 +320,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 +335,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
36 changes: 14 additions & 22 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 @@ -216,26 +216,28 @@ - (void)handleAuthReplyWithSuccess:(BOOL)success
result(@YES);
} else {
switch (error.code) {
case LAErrorPasscodeNotSet:
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;
} else {
result(@NO);
}
return;
#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.)

// 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.
case LAErrorTouchIDNotAvailable:
case LAErrorTouchIDNotEnrolled:
case LAErrorTouchIDLockout:
#pragma clang diagnostic pop
case LAErrorUserFallback:
case LAErrorPasscodeNotSet:
case LAErrorAuthenticationFailed:
[self handleErrors:error flutterArguments:arguments withFlutterResult:result];
return;
case LAErrorSystemCancel:
if ([arguments[@"stickyAuth"] boolValue]) {
self->_lastCallArgs = arguments;
self->_lastResult = result;
return;
}
}
result(@NO);
}
}

Expand All @@ -245,12 +247,7 @@ - (void)handleErrors:(NSError *)authError
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 +257,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
2 changes: 1 addition & 1 deletion packages/local_auth/local_auth_ios/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: local_auth_ios
description: iOS implementation of the local_auth plugin.
repository: https://github.com/flutter/plugins/tree/main/packages/local_auth/local_auth_ios
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+local_auth%22
version: 1.0.10
version: 1.0.11

environment:
sdk: ">=2.14.0 <3.0.0"
Expand Down