-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[local_auth] Differentiate iOS authentication errors #9705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request enhances iOS authentication error handling by mapping native LAError codes to more specific PlatformExceptions, improving the developer experience. The changes are well-structured and include new test cases for the added error conditions. I've provided feedback to improve code clarity and maintainability, primarily focusing on code style and formatting in the generated files, as well as enum ordering.
LouiseHsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick skim and it looks pretty good, but I think this PR might need tests added to https://github.com/flutter/packages/blob/main/packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift ?
|
Changing the codes of existing exceptions is potentially client-breaking. Is there a compelling reason this needs to be done as a semi-breaking change just for iOS, still using the anti-pattern of requiring clients to do string matching against magic, undocumented string values, rather than doing a full breaking change to correctly fix this across all implemenations? |
|
@stuartmorgan-g Please can you clarify what you mean ? I don't think i altered any original exceptions - I only added more exceptions |
How so? The PR description lists three cases where a specific case used to have one exception code, and now has a different code. Anyone who has code trying to handle those cases by matching the exception code string (e.g., by providing a list of possible problems to the user) will now not have that code triggered. It's true that the old situation wasn't good and this allows for better handler targeting going forward, but this change could still require people to update their code, and it seems less than ideal to do that twice (once with this change, and then again later to use consistent, structured exceptions) rather than going directly to the final state. |
|
@stuartmorgan-g I appreciate you taking the time to clarify. I apologize my PR description was not clear (i have updated it). My approach is that since so many different errors were all funneled into the generic NotAvailable code, I am simply adding new, more specific exceptions(there were no previous implementations of these exceptions) However, I now understand your point that this is a semi-breaking change in the sense that developers who were previously handling a user cancellation by checking for e.code == 'NotAvailable' would find that their logic no longer triggers after this change, as they would now need to check for the new, more specific code. |
LouiseHsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a minor comment on the changelog, but otherwise lgtm!
packages/local_auth/local_auth_darwin/example/ios/Runner.xcodeproj/project.pbxproj
Show resolved
Hide resolved
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
flutter/packages@83ae18a...f0b2726 2025-08-06 [email protected] [local_auth] Differentiate iOS authentication errors (flutter/packages#9705) 2025-08-06 [email protected] manual roll to 59fc766 (flutter/packages#9758) 2025-08-05 [email protected] [ci] Exclude packages using deprecated APIs from the pod linter check (flutter/packages#9746) 2025-08-05 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.5.1 to 8.11.1 in /packages/file_selector/file_selector_android/android (flutter/packages#9621) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
* main: (56 commits) [go_router_builder] Support extension types (flutter#9458) Roll Flutter from e2a347b14a18 to 34c2a3b158b2 (41 revisions) (flutter#9803) [go_router_builder] Migrate to Element2 API and update dependencies (flutter#9649) [in_app_purchase_storekit] Add support for quantity in consumable product purchases (#171570) (flutter#9698) [pigeon] Improves documentation of `ProxyApi` and moves helper functions to a separate file (flutter#9756) Roll Flutter from 1590543f6794 to e2a347b14a18 (1 revision) (flutter#9784) [camera_avfoundation] Implementation swift migration - part 11 (flutter#9690) [camera_avfoundation] Fix crash when streaming while recording (flutter#9691) Roll Flutter from 38217906e95c to 1590543f6794 (14 revisions) (flutter#9780) Roll Flutter from 92a6bfbfd6ef to 38217906e95c (17 revisions) (flutter#9778) [video_player] Improve KVO handling on iOS (flutter#9718) [dependabot]: Bump the test-dependencies group across 15 directories with 7 updates (flutter#9736) Roll Flutter from 9de63a03428f to 92a6bfbfd6ef (11 revisions) (flutter#9769) [google_maps_flutter_platform_interface] Add Advanced markers support (flutter#9737) [camera_android_camerax] Re-land "Force new Surface for each SurfaceRequest" (flutter#9760) [google_maps_flutter] Add ability to perform Google Maps SDK warmup (flutter#9674) Roll Flutter from 59fc766c6fdf to 9de63a03428f (6 revisions) (flutter#9764) [webview_flutter_wkwebview] Extended Web View API on iOS to add flexibility when working with local HTML content (flutter#8787) [local_auth] Differentiate iOS authentication errors (flutter#9705) manual roll to 59fc766c6fdfd03d0983fc95ce8b76793a300dd5 (flutter#9758) ... # Conflicts: # packages/go_router_builder/lib/src/type_helpers.dart # packages/go_router_builder/pubspec.yaml
flutter/packages@83ae18a...f0b2726 2025-08-06 [email protected] [local_auth] Differentiate iOS authentication errors (flutter/packages#9705) 2025-08-06 [email protected] manual roll to 59fc766 (flutter/packages#9758) 2025-08-05 [email protected] [ci] Exclude packages using deprecated APIs from the pod linter check (flutter/packages#9746) 2025-08-05 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.5.1 to 8.11.1 in /packages/file_selector/file_selector_android/android (flutter/packages#9621) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter/packages@83ae18a...f0b2726 2025-08-06 [email protected] [local_auth] Differentiate iOS authentication errors (flutter/packages#9705) 2025-08-06 [email protected] manual roll to 59fc766 (flutter/packages#9758) 2025-08-05 [email protected] [ci] Exclude packages using deprecated APIs from the pod linter check (flutter/packages#9746) 2025-08-05 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.5.1 to 8.11.1 in /packages/file_selector/file_selector_android/android (flutter/packages#9621) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter/packages@83ae18a...f0b2726 2025-08-06 [email protected] [local_auth] Differentiate iOS authentication errors (flutter/packages#9705) 2025-08-06 [email protected] manual roll to 59fc766 (flutter/packages#9758) 2025-08-05 [email protected] [ci] Exclude packages using deprecated APIs from the pod linter check (flutter/packages#9746) 2025-08-05 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.5.1 to 8.11.1 in /packages/file_selector/file_selector_android/android (flutter/packages#9621) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter/packages@83ae18a...f0b2726 2025-08-06 [email protected] [local_auth] Differentiate iOS authentication errors (flutter/packages#9705) 2025-08-06 [email protected] manual roll to 59fc766 (flutter/packages#9758) 2025-08-05 [email protected] [ci] Exclude packages using deprecated APIs from the pod linter check (flutter/packages#9746) 2025-08-05 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.5.1 to 8.11.1 in /packages/file_selector/file_selector_android/android (flutter/packages#9621) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
On iOS, the plugin previously returned a generic
NotAvailablePlatformException for several distinct failure conditions. This made
it impossible for developers to handle specific cases e.g user
cancellation.
This change ensures the plugin returns specific error codes for distinct user actions on iOS:
Fixes flutter/flutter#148942
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3