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

[local_auth] Fix iOS crash when no localizedReason #3780

Merged
merged 7 commits into from
Apr 9, 2021
Merged

[local_auth] Fix iOS crash when no localizedReason #3780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25bead8
Fix iOS crash when no localizedReason
enricobenedos Apr 2, 2021
64db76c
Updated versioning files
enricobenedos Apr 2, 2021
52bfb1d
Removed null assertion due to null safety
enricobenedos Apr 8, 2021
60c4e52
Updated authenticate() method documnetation
enricobenedos Apr 8, 2021
4f83842
Merge branch 'master' into local_auth_app_crash
Apr 8, 2021
2f8a09a
Update packages/local_auth/CHANGELOG.md
enricobenedos Apr 9, 2021
8e3fe6e
Update packages/local_auth/lib/local_auth.dart
enricobenedos Apr 9, 2021
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/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.1.4

* Add debug assertion that `localizedReason` in `LocalAuthentication.authenticateWithBiometrics` must not be empty.

## 1.1.3

* Fix crashes due to threading issues in iOS implementation.
Expand Down
5 changes: 3 additions & 2 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 @@ -59,7 +59,7 @@ class LocalAuthentication {
///
/// [localizedReason] is the message to show to user while prompting them
/// for authentication. This is typically along the lines of: 'Please scan
/// your finger to access MyApp.'
/// your finger to access MyApp.'. This must not be empty.
///
/// [useErrorDialogs] = true means the system will attempt to handle user
/// fixable issues encountered while authenticating. For instance, if
Expand Down Expand Up @@ -100,7 +100,8 @@ class LocalAuthentication {
bool sensitiveTransaction = true,
bool biometricOnly = false,
}) async {
assert(localizedReason != null);
assert(localizedReason.isNotEmpty);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the behavior on android if this is empty?

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 Android 11 (simulator) there are no problems if the localizedReason is empty. Is it a good idea to add the assert only on iOS case using Platform.isIOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a good idea to add the assert only on iOS case using Platform.isIOS?

Yes. If android can handle null, especially if null and empty string means different things on android, we should allow null on android. Do you know if there are difference between null and empty string on android?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again. I think you are right, we really shouldn't allow empty strings or null on android either. So the change looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

@enricobenedos Let's make sure to mention the localizedReason must not be an empty String in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @cyanglaz, sorry for the delay but I'm a little bit busy. I'm happy that you check that is it a good idea to also not give the possibility to input an empty string on Android. I will update the docs soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks, once you update the doc we will land this!

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've updated the authenticate method documentation.


final Map<String, Object> args = <String, Object>{
'localizedReason': localizedReason,
'useErrorDialogs': useErrorDialogs,
Expand Down
2 changes: 1 addition & 1 deletion packages/local_auth/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
description: Flutter plugin for Android and iOS devices to allow local
authentication via fingerprint, touch ID, face ID, passcode, pin, or pattern.
homepage: https://github.com/flutter/plugins/tree/master/packages/local_auth
version: 1.1.3
version: 1.1.4

flutter:
plugin:
Expand Down
11 changes: 11 additions & 0 deletions packages/local_auth/test/local_auth_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,17 @@ void main() {
);
});

test('authenticate with no localizedReason on iOS.', () async {
setMockPathProviderPlatform(FakePlatform(operatingSystem: 'ios'));
await expectLater(
localAuthentication.authenticate(
localizedReason: '',
biometricOnly: true,
),
throwsAssertionError,
);
});

test('authenticate with no sensitive transaction.', () async {
setMockPathProviderPlatform(FakePlatform(operatingSystem: 'android'));
await localAuthentication.authenticate(
Expand Down