From a3cd9e7ede34d6eaacf0dd743c7d6e2c00d85884 Mon Sep 17 00:00:00 2001 From: Dillon Nys Date: Mon, 4 Sep 2023 16:37:07 -0700 Subject: [PATCH 1/2] chore(auth): Use `EventCompleter.ignore` instead of `unawaited` `unawaited` will still raise an error if there is one. By using `EventCompleter.ignore` we drop all completions, success or error. --- .../lib/src/state_machine/event.dart | 13 ++++++++----- .../lib/src/state_machine/state_machine.dart | 2 ++ .../hosted_ui/hosted_ui_platform_flutter.dart | 8 ++++---- .../flows/hosted_ui/hosted_ui_platform_io.dart | 18 +++++++++--------- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/packages/amplify_core/lib/src/state_machine/event.dart b/packages/amplify_core/lib/src/state_machine/event.dart index 6e37e90588..b06976098d 100644 --- a/packages/amplify_core/lib/src/state_machine/event.dart +++ b/packages/amplify_core/lib/src/state_machine/event.dart @@ -68,10 +68,10 @@ final class EventCompleter completer.completeError(error))` would + /// still throw the error in the Zone where the completer was instantiated, + /// not `_zone`. And due to how Zone's work, a listener for a completer which + /// completes in a different error zone will never finish. /// /// The following example illustrates the problem we're trying to solve /// here: @@ -168,5 +168,8 @@ final class EventCompleter Date: Mon, 4 Sep 2023 16:44:53 -0700 Subject: [PATCH 2/2] fix(auth): Uncaught Hosted UI cancellation The HostedUIPlatform had a bug due to the fact that it both called `dispatcher.dispatchAndComplete` (which throws on failure states) and throwing a `UserCancelledException` which triggers a failure state. This lead to the error being thrown from `signInWithWebUI` and an uncaught exception being raised at the `dispatchAndComplete` call. This resolves the issue by only throwing from the HostedUiPlatform and letting the state machine resolving the cancellation. This also fixes an issue where `cancelSignIn` was never being called because the state machine was in a `signingIn` state at the time the cancellation was dispatched. --- .../hosted_ui_webview_test.dart | 58 ++++++++++-- .../amplify_auth_cognito/example/pubspec.yaml | 1 + .../hosted_ui/hosted_ui_platform_flutter.dart | 59 ++++++------ .../auth/amplify_auth_cognito/pubspec.yaml | 1 + .../test/hosted_ui_platform_flutter_test.dart | 90 +++++++++++++++++++ .../lib/src/state/event/hosted_ui_event.dart | 22 ++++- .../machines/hosted_ui_state_machine.dart | 18 ++-- .../hostedui/hosted_ui_platform_test.dart | 77 ++++++++++++++++ 8 files changed, 284 insertions(+), 42 deletions(-) create mode 100644 packages/auth/amplify_auth_cognito/test/hosted_ui_platform_flutter_test.dart diff --git a/packages/auth/amplify_auth_cognito/example/integration_test/hosted_ui_webview_test.dart b/packages/auth/amplify_auth_cognito/example/integration_test/hosted_ui_webview_test.dart index 5f33b31d92..b3aa7fea8d 100644 --- a/packages/auth/amplify_auth_cognito/example/integration_test/hosted_ui_webview_test.dart +++ b/packages/auth/amplify_auth_cognito/example/integration_test/hosted_ui_webview_test.dart @@ -13,7 +13,6 @@ import 'package:amplify_auth_cognito_dart/src/flows/hosted_ui/hosted_ui_platform import 'package:amplify_auth_cognito_dart/src/model/hosted_ui/oauth_parameters.dart'; import 'package:amplify_auth_cognito_dart/src/state/cognito_state_machine.dart'; import 'package:amplify_auth_cognito_dart/src/state/state.dart'; -import 'package:amplify_auth_cognito_test/amplify_auth_cognito_test.dart'; import 'package:amplify_flutter/amplify_flutter.dart'; import 'package:amplify_integration_test/amplify_integration_test.dart'; import 'package:flutter/material.dart'; @@ -56,21 +55,52 @@ void main() { ); }); - Future signIn(WidgetTester tester) async { + Future signIn( + WidgetTester tester, { + bool cancel = false, + }) async { stateMachine.addInstance( HostedUiTestPlatform( tester, stateMachine, username: username, password: password, + cancel: cancel, ), ); _logger.debug('Signing in with Web UI'); - final result = await plugin.signInWithWebUI( - provider: AuthProvider.cognito, - ); - _logger.debug('Signed in with Web UI'); - expect(result.isSignedIn, isTrue); + if (cancel) { + final expectation = expectLater( + plugin.signInWithWebUI( + provider: AuthProvider.cognito, + ), + throwsA(isA()), + ); + final hostedUiMachine = + stateMachine.expect(HostedUiStateMachine.type); + expect( + hostedUiMachine.stream, + emitsInOrder([ + isA(), + isA().having( + (s) => s.exception, + 'exception', + isA(), + ), + emitsDone, + ]), + ); + await expectation; + // Ensure queue is flushed and done event is emitted after + // signInWithWebUI completes. + await hostedUiMachine.close(); + } else { + final result = await plugin.signInWithWebUI( + provider: AuthProvider.cognito, + ); + _logger.debug('Signed in with Web UI'); + expect(result.isSignedIn, isTrue); + } } Future signOut({required bool globalSignOut}) async { @@ -112,14 +142,17 @@ void main() { await signOut(globalSignOut: false); }); + testWidgets('cancel sign-in', (tester) async { + await signIn(tester, cancel: true); + }); + testWidgets('global sign out', (tester) async { await signIn(tester); await signOut(globalSignOut: true); }); }, // Add remaining platforms as `webview_flutter` adds support. - // TODO(dnys1): Investigate Android failures in CI on Android 33+ - skip: zIsWeb || !((Platform.isAndroid && !isCI) || Platform.isIOS), + skip: zIsWeb || !(Platform.isAndroid || Platform.isIOS), ); } @@ -129,11 +162,15 @@ class HostedUiTestPlatform extends HostedUiPlatformImpl { DependencyManager manager, { required this.username, required this.password, + required this.cancel, }) : super(manager); final String username; final String password; + /// Whether to cancel the sign-in flow once initiated. + final bool cancel; + final WidgetTester tester; final Completer _controller = Completer(); final Completer _oauthParameters = Completer(); @@ -143,6 +180,9 @@ class HostedUiTestPlatform extends HostedUiPlatformImpl { required CognitoSignInWithWebUIPluginOptions options, AuthProvider? provider, }) async { + if (cancel) { + throw const UserCancelledException('Cancelled'); + } final signInUri = await getSignInUri(provider: provider); await tester.pumpWidget( HostedUiApp( diff --git a/packages/auth/amplify_auth_cognito/example/pubspec.yaml b/packages/auth/amplify_auth_cognito/example/pubspec.yaml index 96bae49ee7..ddc36409fe 100644 --- a/packages/auth/amplify_auth_cognito/example/pubspec.yaml +++ b/packages/auth/amplify_auth_cognito/example/pubspec.yaml @@ -40,6 +40,7 @@ dev_dependencies: io: ^1.0.0 otp: ^3.1.4 stack_trace: ^1.10.0 + stream_transform: ^2.1.0 test: ^1.22.1 webdriver: ^3.0.0 webview_flutter: ^4.0.0 diff --git a/packages/auth/amplify_auth_cognito/lib/src/flows/hosted_ui/hosted_ui_platform_flutter.dart b/packages/auth/amplify_auth_cognito/lib/src/flows/hosted_ui/hosted_ui_platform_flutter.dart index b7c55c341e..f108e5110e 100644 --- a/packages/auth/amplify_auth_cognito/lib/src/flows/hosted_ui/hosted_ui_platform_flutter.dart +++ b/packages/auth/amplify_auth_cognito/lib/src/flows/hosted_ui/hosted_ui_platform_flutter.dart @@ -16,6 +16,7 @@ import 'package:amplify_auth_cognito_dart/src/model/hosted_ui/oauth_parameters.d // ignore: implementation_imports, invalid_use_of_internal_member import 'package:amplify_auth_cognito_dart/src/state/state.dart'; import 'package:amplify_core/amplify_core.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/services.dart'; /// {@template amplify_auth_cognito.hosted_ui_platform_flutter} @@ -27,7 +28,17 @@ class HostedUiPlatformImpl extends io.HostedUiPlatformImpl { /// {@macro amplify_auth_cognito.hosted_ui_platform_flutter} HostedUiPlatformImpl(super.dependencyManager); - static final bool _isMobile = Platform.isAndroid || Platform.isIOS; + static bool get _isMobile { + if (Platform.isAndroid || Platform.isIOS) { + return true; + } + // Allow overrides for tests + if (zAssertsEnabled) { + return debugDefaultTargetPlatformOverride == TargetPlatform.android || + debugDefaultTargetPlatformOverride == TargetPlatform.iOS; + } + return false; + } NativeAuthBridge get _nativeAuthBridge => dependencyManager.expect(); @@ -82,37 +93,35 @@ class HostedUiPlatformImpl extends io.HostedUiPlatformImpl { ); dispatcher .dispatch( - HostedUiEvent.exchange( - OAuthParameters.fromJson(queryParameters.cast()), - ), + HostedUiEvent.exchange( + OAuthParameters.fromJson(queryParameters.cast()), + ), ) .ignore(); } on Exception catch (e) { - unawaited( - dispatcher.dispatchAndComplete(const HostedUiEvent.cancelSignIn()), - ); - if (e is PlatformException) { - if (e.code == 'CANCELLED') { + switch (e) { + case PlatformException(code: 'CANCELLED'): throw const UserCancelledException( 'The user cancelled the sign-in flow', ); - } - // Generated Android message is `CLASS_NAME: message` - var message = e.message; - if (message != null && message.contains(': ')) { - message = message.split(': ')[1]; - } - String? recoverySuggestion; - if (e.code == 'NOBROWSER') { - recoverySuggestion = "Ensure you've added the tag to your " - 'AndroidManifest.xml as outlined in the docs'; - } - throw UrlLauncherException( - message ?? 'An unknown error occurred', - recoverySuggestion: recoverySuggestion, - ); + case PlatformException(:final code, :var message): + // Generated Android message is `CLASS_NAME: message` + if (message != null && message.contains(': ')) { + message = message.split(': ')[1]; + } + String? recoverySuggestion; + if (code == 'NOBROWSER') { + recoverySuggestion = + "Ensure you've added the tag to your " + 'AndroidManifest.xml as outlined in the docs'; + } + throw UrlLauncherException( + message ?? 'An unknown error occurred', + recoverySuggestion: recoverySuggestion, + ); + default: + rethrow; } - rethrow; } } diff --git a/packages/auth/amplify_auth_cognito/pubspec.yaml b/packages/auth/amplify_auth_cognito/pubspec.yaml index 20790eda9b..8042e86754 100644 --- a/packages/auth/amplify_auth_cognito/pubspec.yaml +++ b/packages/auth/amplify_auth_cognito/pubspec.yaml @@ -33,6 +33,7 @@ dependencies: plugin_platform_interface: ^2.0.0 dev_dependencies: + amplify_auth_cognito_test: any amplify_lints: ">=3.0.0 <3.1.0" flutter_test: sdk: flutter diff --git a/packages/auth/amplify_auth_cognito/test/hosted_ui_platform_flutter_test.dart b/packages/auth/amplify_auth_cognito/test/hosted_ui_platform_flutter_test.dart new file mode 100644 index 0000000000..728f725c04 --- /dev/null +++ b/packages/auth/amplify_auth_cognito/test/hosted_ui_platform_flutter_test.dart @@ -0,0 +1,90 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// ignore_for_file: invalid_use_of_internal_member, non_constant_identifier_names + +import 'package:amplify_auth_cognito/amplify_auth_cognito.dart'; +import 'package:amplify_auth_cognito/src/flows/hosted_ui/hosted_ui_platform_flutter.dart'; +import 'package:amplify_auth_cognito/src/native_auth_plugin.g.dart'; +import 'package:amplify_auth_cognito_dart/src/flows/hosted_ui/hosted_ui_platform.dart'; +import 'package:amplify_auth_cognito_dart/src/state/cognito_state_machine.dart'; +import 'package:amplify_auth_cognito_dart/src/state/state.dart'; +import 'package:amplify_auth_cognito_test/amplify_auth_cognito_test.dart'; +import 'package:amplify_flutter/amplify_flutter.dart'; +import 'package:flutter/foundation.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + AWSLogger().logLevel = LogLevel.verbose; + + group('HostedUiPlatformFlutter', () { + late AmplifyAuthCognito plugin; + late SecureStorageInterface secureStorage; + late DependencyManager dependencyManager; + + setUp(() async { + secureStorage = MockSecureStorage(); + dependencyManager = DependencyManager() + ..addInstance(hostedUiConfig) + ..addInstance(secureStorage) + ..addInstance(ThrowingNativeBridge()); + plugin = AmplifyAuthCognito() + ..stateMachine = CognitoAuthStateMachine( + dependencyManager: dependencyManager, + ); + plugin.stateMachine.addBuilder( + HostedUiPlatformImpl.new, + ); + await plugin.stateMachine.acceptAndComplete( + ConfigurationEvent.configure(mockConfig), + ); + }); + + tearDown(() => plugin.close()); + + test('can cancel flow', () async { + // Pretend to be iOS so that the `ThrowingNativeBridge` is called, + // mimicking a failure from the platform channel. + debugDefaultTargetPlatformOverride = TargetPlatform.iOS; + addTearDown(() => debugDefaultTargetPlatformOverride = null); + + final expectation = expectLater( + plugin.signInWithWebUI( + provider: AuthProvider.cognito, + ), + throwsA(isA()), + ); + final hostedUiMachine = + plugin.stateMachine.expect(HostedUiStateMachine.type); + expect( + hostedUiMachine.stream, + emitsInOrder([ + isA(), + isA().having( + (s) => s.exception, + 'exception', + isA(), + ), + emitsDone, + ]), + ); + await expectation; + // Ensure queue is flushed and done event is emitted after + // signInWithWebUI completes. + await hostedUiMachine.close(); + }); + }); +} + +final class ThrowingNativeBridge extends Fake implements NativeAuthBridge { + @override + Future> signInWithUrl( + String arg_url, + String arg_callbackUrlScheme, + bool arg_preferPrivateSession, + String? arg_browserPackageName, + ) async { + throw PlatformException(code: 'CANCELLED'); + } +} diff --git a/packages/auth/amplify_auth_cognito_dart/lib/src/state/event/hosted_ui_event.dart b/packages/auth/amplify_auth_cognito_dart/lib/src/state/event/hosted_ui_event.dart index 1e04fcbbf0..0c6d2a9e4d 100644 --- a/packages/auth/amplify_auth_cognito_dart/lib/src/state/event/hosted_ui_event.dart +++ b/packages/auth/amplify_auth_cognito_dart/lib/src/state/event/hosted_ui_event.dart @@ -49,7 +49,10 @@ sealed class HostedUiEvent }) = HostedUiSignIn; /// {@macro amplify_auth_cognito.hosted_ui_cancel_sign_in} - const factory HostedUiEvent.cancelSignIn() = HostedUiCancelSignIn; + const factory HostedUiEvent.cancelSignIn([ + Object? error, + StackTrace? stackTrace, + ]) = HostedUiCancelSignIn; /// {@macro amplify_auth_cognito.hosted_ui_exchange} const factory HostedUiEvent.exchange(OAuthParameters parameters) = @@ -143,10 +146,23 @@ final class HostedUiSignIn extends HostedUiEvent { /// {@endtemplate} final class HostedUiCancelSignIn extends HostedUiEvent { /// {@macro amplify_auth_cognito.hosted_ui_cancel_sign_in} - const HostedUiCancelSignIn() : super._(); + const HostedUiCancelSignIn([ + Object? error, + this.stackTrace, + ]) : error = error ?? + const UserCancelledException( + 'The user canceled the sign-in flow', + ), + super._(); + + /// The cause of the cancellation. + final Object error; + + /// The stack trace associated with [error]. + final StackTrace? stackTrace; @override - List get props => [type]; + List get props => [type, error, stackTrace]; @override HostedUiEventType get type => HostedUiEventType.cancelSignIn; diff --git a/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/hosted_ui_state_machine.dart b/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/hosted_ui_state_machine.dart index 655ca454f6..b9235e84ee 100644 --- a/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/hosted_ui_state_machine.dart +++ b/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/hosted_ui_state_machine.dart @@ -36,7 +36,7 @@ final class HostedUiStateMachine SecureStorageInterface get _secureStorage => getOrCreate(); /// The platform-specific behavior. - late final HostedUiPlatform _platform = getOrCreate(); + HostedUiPlatform get _platform => getOrCreate(); /// The configured identity pool. CognitoIdentityCredentialsProvider? get _identityPoolConfig => get(); @@ -128,9 +128,14 @@ final class HostedUiStateMachine } else { await _secureStorage.delete(key: _keys[HostedUiKey.provider]); } - await _platform.signIn( - options: event.options, - provider: provider, + unawaited( + // Unblock the state machine to accept new events but ensure + // that [onCancelSignIn] is called if any error occurs. + _platform.signIn(options: event.options, provider: provider).onError( + (error, stackTrace) => dispatch( + HostedUiEvent.cancelSignIn(error, stackTrace), + ), + ), ); } @@ -138,7 +143,10 @@ final class HostedUiStateMachine Future onCancelSignIn(HostedUiCancelSignIn event) async { await _platform.cancelSignIn(); await manager.clearCredentials(_keys); - throw const UserCancelledException('The user cancelled the sign-in flow'); + Error.throwWithStackTrace( + event.error, + event.stackTrace ?? StackTrace.current, + ); } /// State machine callback for the [HostedUiExchange] event. diff --git a/packages/auth/amplify_auth_cognito_test/test/flows/hostedui/hosted_ui_platform_test.dart b/packages/auth/amplify_auth_cognito_test/test/flows/hostedui/hosted_ui_platform_test.dart index e5301c1d9a..c9a600d928 100644 --- a/packages/auth/amplify_auth_cognito_test/test/flows/hostedui/hosted_ui_platform_test.dart +++ b/packages/auth/amplify_auth_cognito_test/test/flows/hostedui/hosted_ui_platform_test.dart @@ -1,9 +1,13 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +import 'dart:async'; + +import 'package:amplify_auth_cognito_dart/amplify_auth_cognito_dart.dart'; import 'package:amplify_auth_cognito_dart/src/credentials/cognito_keys.dart'; import 'package:amplify_auth_cognito_dart/src/flows/hosted_ui/hosted_ui_platform.dart'; import 'package:amplify_auth_cognito_dart/src/model/hosted_ui/oauth_parameters.dart'; +import 'package:amplify_auth_cognito_dart/src/state/cognito_state_machine.dart'; import 'package:amplify_auth_cognito_dart/src/state/state.dart'; import 'package:amplify_auth_cognito_test/common/mock_config.dart'; import 'package:amplify_auth_cognito_test/common/mock_dispatcher.dart'; @@ -12,6 +16,7 @@ import 'package:amplify_auth_cognito_test/common/mock_secure_storage.dart'; import 'package:amplify_core/amplify_core.dart'; import 'package:amplify_secure_storage_dart/amplify_secure_storage_dart.dart'; import 'package:http/http.dart' as http; +import 'package:test/fake.dart'; import 'package:test/test.dart'; final throwsInvalidStateException = throwsA(isA()); @@ -23,6 +28,8 @@ void main() { late DependencyManager dependencyManager; const keys = HostedUiKeys(hostedUiConfig); + AWSLogger().logLevel = LogLevel.verbose; + group('HostedUiPlatform', () { setUp(() { server = MockOAuthServer(); @@ -108,5 +115,75 @@ void main() { ); }); }); + + group('signIn', () { + late AmplifyAuthCognitoDart plugin; + + setUp(() async { + dependencyManager.addInstance( + CancelingHostedUiPlatform( + cancelSignIn: expectAsync0(() async {}), + ), + ); + plugin = AmplifyAuthCognitoDart() + ..stateMachine = CognitoAuthStateMachine( + dependencyManager: dependencyManager, + ); + await plugin.stateMachine.acceptAndComplete( + ConfigurationEvent.configure(mockConfig), + ); + }); + + tearDown(() => plugin.close()); + + test('can cancel flow', () async { + final expectation = expectLater( + plugin.signInWithWebUI( + provider: AuthProvider.cognito, + ), + throwsA(isA()), + ); + final hostedUiMachine = + plugin.stateMachine.expect(HostedUiStateMachine.type); + expect( + hostedUiMachine.stream, + emitsInOrder([ + isA(), + isA().having( + (s) => s.exception, + 'exception', + isA(), + ), + emitsDone, + ]), + ); + await expectation; + // Ensure queue is flushed and done event is emitted after + // signInWithWebUI completes. + await hostedUiMachine.close(); + }); + }); }); } + +final class CancelingHostedUiPlatform extends Fake implements HostedUiPlatform { + CancelingHostedUiPlatform({ + required Future Function() cancelSignIn, + }) : _cancelSignIn = cancelSignIn; + + final Future Function() _cancelSignIn; + + @override + Future signIn({ + required CognitoSignInWithWebUIPluginOptions options, + AuthProvider? provider, + }) async { + throw const UserCancelledException('Cancelled'); + } + + @override + Future cancelSignIn() => _cancelSignIn(); + + @override + void close() {} +}