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

[local_auth] Fix iOS crash when no localizedReason #3780

Merged
fluttergithubbot 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 2 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.2

* Fixed iOS app crash when fill `localizedReason` with an empty string

## 1.1.1

* Update flutter_plugin_android_lifecycle dependency to 2.0.1 to fix an R8 issue
Expand Down
2 changes: 2 additions & 0 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 @@ -101,6 +101,8 @@ class LocalAuthentication {
bool biometricOnly = false,
}) async {
assert(localizedReason != null);
Copy link
Contributor

@cyanglaz cyanglaz Apr 2, 2021

Choose a reason for hiding this comment

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

Since this plugin has migrated to null safety, this line is actually not necessary anymore. Feel free to remove it while you are here.

Copy link
Contributor Author

@enricobenedos enricobenedos Apr 8, 2021

Choose a reason for hiding this comment

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

👍🏼

Copy link
Contributor Author

@enricobenedos enricobenedos Apr 8, 2021

Choose a reason for hiding this comment

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

Done

assert(localizedReason.isNotEmpty);
Copy link
Contributor

@cyanglaz cyanglaz Apr 2, 2021

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

@enricobenedos enricobenedos Apr 4, 2021

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

@cyanglaz cyanglaz Apr 5, 2021

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

@cyanglaz cyanglaz Apr 8, 2021

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

@cyanglaz cyanglaz Apr 8, 2021

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

@enricobenedos enricobenedos Apr 8, 2021

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

@cyanglaz cyanglaz Apr 8, 2021

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

@enricobenedos enricobenedos Apr 8, 2021

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.1
version: 1.1.2

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