-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[webview_flutter] Support for handling basic authentication requests (iOS) #5455
[webview_flutter] Support for handling basic authentication requests (iOS) #5455
Conversation
…/packages into webview-auth-request
…b/src/types/http_auth_request.dart
…s into webview-auth-request
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.
Everything looks good. Just need to update the pubspecs
|
||
# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE. | ||
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins | ||
dependency_overrides: | ||
{webview_flutter_platform_interface: {path: ../../../webview_flutter/webview_flutter_platform_interface}} |
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.
Remove this and update the minimum webview_flutter_platform_interface
to ^2.7.0
# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE. | |
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins | |
dependency_overrides: | |
{webview_flutter_platform_interface: {path: ../../../webview_flutter/webview_flutter_platform_interface}} |
|
||
# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE. | ||
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins | ||
dependency_overrides: | ||
{webview_flutter_platform_interface: {path: ../../webview_flutter/webview_flutter_platform_interface}} |
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.
Same for this
# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE. | |
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins | |
dependency_overrides: | |
{webview_flutter_platform_interface: {path: ../../webview_flutter/webview_flutter_platform_interface}} |
…as06/packages into webview-auth-request-ios
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
@stuartmorgan for secondary review
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 with one question.
challenge:challenge | ||
completion:^(FWFAuthenticationChallengeResponse *response, | ||
FlutterError *error) { | ||
NSAssert(!error, @"%@", error); |
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.
Why is this an assert? Couldn't a plugin client have an implementation on the Dart side that throws an exception, leading to a FlutterError
here?
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.
This is a pattern I started with all the FlutterApi methods. IIRC when there is an exception thrown on the Dart side during a FlutterApi method, this error is returned and it wouldn't be printed anywhere. Essentially failing silently. This would catch the error and print it to the console when the app is not in release mode or disabling asserts. It's mainly to help with debugging.
At the very least, this used to be the case on Android.
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.
Right, but in most cases I thought it was just calling our own code, so we could reasonably assert that an error didn't exist.
I guess the question here is: if a plugin client throws an exception in their own Dart code that implements the handler, do we want the entire application to crash?
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.
@bparrishMines Should we adjust this, or land as-is and revisit later if necessary?
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 created flutter/flutter#140410 to track the discussion. I think we could probably find a better alternative, but I haven't seen any issues raised by users.
flutter/packages@be52ac8...dc5b267 2023-12-20 [email protected] Roll Flutter from 0eb7881 to da0cd69 (15 revisions) (flutter/packages#5729) 2023-12-20 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump androidx.annotation:annotation from 1.7.0 to 1.7.1 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#5702) 2023-12-19 [email protected] [webview_flutter] Implement platform interface for JavaScript dialog (flutter/packages#5670) 2023-12-19 [email protected] [webview_flutter] Support for handling basic authentication requests (iOS) (flutter/packages#5455) 2023-12-19 [email protected] Roll Flutter from cdc83e5 to 0eb7881 (17 revisions) (flutter/packages#5722) 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],[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
…(iOS) (flutter#5455) Adds the iOS implementation for basic http authentication. This PR is part of a series of PRs that aim to close flutter/flutter#83556. The PR that contains all changes can be found at flutter#4140.
flutter/packages@be52ac8...dc5b267 2023-12-20 [email protected] Roll Flutter from 0eb7881 to da0cd69 (15 revisions) (flutter/packages#5729) 2023-12-20 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump androidx.annotation:annotation from 1.7.0 to 1.7.1 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#5702) 2023-12-19 [email protected] [webview_flutter] Implement platform interface for JavaScript dialog (flutter/packages#5670) 2023-12-19 [email protected] [webview_flutter] Support for handling basic authentication requests (iOS) (flutter/packages#5455) 2023-12-19 [email protected] Roll Flutter from cdc83e5 to 0eb7881 (17 revisions) (flutter/packages#5722) 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],[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
…5727) ## Description This pull request exposes the Android and iOS HTTP Basic Authentication feature to users of the `webview_flutter` plugin. It is the final PR in a sequence of PRs. Previous PRs are #5362, #5454 and #5455. Issues fixed by PR: Closes flutter/flutter#83556
…(iOS) (flutter#5455) Adds the iOS implementation for basic http authentication. This PR is part of a series of PRs that aim to close flutter/flutter#83556. The PR that contains all changes can be found at flutter#4140.
…lutter#5727) ## Description This pull request exposes the Android and iOS HTTP Basic Authentication feature to users of the `webview_flutter` plugin. It is the final PR in a sequence of PRs. Previous PRs are flutter#5362, flutter#5454 and flutter#5455. Issues fixed by PR: Closes flutter/flutter#83556
Adds the iOS implementation for basic http authentication.
This PR is part of a series of PRs that aim to close flutter/flutter#83556.
The PR that contains all changes can be found at #4140.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).