Skip to content

Commit

Permalink
fix(auth): Uncaught Hosted UI cancellation (#3686)
Browse files Browse the repository at this point in the history
* 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.

* 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.
  • Loading branch information
dnys1 authored Sep 5, 2023
1 parent 8770e1e commit 802c7c8
Show file tree
Hide file tree
Showing 11 changed files with 307 additions and 60 deletions.
13 changes: 8 additions & 5 deletions packages/amplify_core/lib/src/state_machine/event.dart
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ final class EventCompleter<Event extends StateMachineEvent,
/// match the Zone in which its future is listened to, otherwise the future
/// will never complete.
///
/// That is, running `_zone.run(completer.complete)` would still throw
/// the error in the Zone where the completer was instantiated. And due
/// to how Zone's work, a listener for a completer which completes in a
/// different error zone will never finish.
/// That is, running `_zone.run(() => 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:
Expand Down Expand Up @@ -168,5 +168,8 @@ final class EventCompleter<Event extends StateMachineEvent,
///
/// Since state machine methods are marked with `@useResult`, this allows
/// opting into fire-and-forget behavior explicitly.
void ignore() {}
void ignore() {
_acceptedCompleters.clear();
_completers.clear();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ abstract class StateMachine<

final resolution = resolveError(error, stackTrace);

logger.debug('Resolved error with state', resolution?.type);

// Add the error to the state stream if it cannot be resolved to a new
// state internally.
if (resolution == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -56,21 +55,52 @@ void main() {
);
});

Future<void> signIn(WidgetTester tester) async {
Future<void> signIn(
WidgetTester tester, {
bool cancel = false,
}) async {
stateMachine.addInstance<HostedUiPlatform>(
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<UserCancelledException>()),
);
final hostedUiMachine =
stateMachine.expect(HostedUiStateMachine.type);
expect(
hostedUiMachine.stream,
emitsInOrder([
isA<HostedUiSigningIn>(),
isA<HostedUiFailure>().having(
(s) => s.exception,
'exception',
isA<UserCancelledException>(),
),
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<void> signOut({required bool globalSignOut}) async {
Expand Down Expand Up @@ -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),
);
}

Expand All @@ -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<WebViewController> _controller = Completer();
final Completer<OAuthParameters> _oauthParameters = Completer();
Expand All @@ -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(
Expand Down
1 change: 1 addition & 0 deletions packages/auth/amplify_auth_cognito/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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();

Expand Down Expand Up @@ -80,39 +91,37 @@ class HostedUiPlatformImpl extends io.HostedUiPlatformImpl {
options.isPreferPrivateSession,
options.browserPackageName,
);
unawaited(
dispatcher.dispatchAndComplete(
HostedUiEvent.exchange(
OAuthParameters.fromJson(queryParameters.cast()),
),
),
);
dispatcher
.dispatch(
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 <queries> 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 <queries> tag to your "
'AndroidManifest.xml as outlined in the docs';
}
throw UrlLauncherException(
message ?? 'An unknown error occurred',
recoverySuggestion: recoverySuggestion,
);
default:
rethrow;
}
rethrow;
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/auth/amplify_auth_cognito/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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<SecureStorageInterface>(secureStorage)
..addInstance<NativeAuthBridge>(ThrowingNativeBridge());
plugin = AmplifyAuthCognito()
..stateMachine = CognitoAuthStateMachine(
dependencyManager: dependencyManager,
);
plugin.stateMachine.addBuilder<HostedUiPlatform>(
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<UserCancelledException>()),
);
final hostedUiMachine =
plugin.stateMachine.expect(HostedUiStateMachine.type);
expect(
hostedUiMachine.stream,
emitsInOrder([
isA<HostedUiSigningIn>(),
isA<HostedUiFailure>().having(
(s) => s.exception,
'exception',
isA<UserCancelledException>(),
),
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<Map<String?, String?>> signInWithUrl(
String arg_url,
String arg_callbackUrlScheme,
bool arg_preferPrivateSession,
String? arg_browserPackageName,
) async {
throw PlatformException(code: 'CANCELLED');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -250,13 +250,13 @@ class HostedUiPlatformImpl extends HostedUiPlatform {
);
continue;
}
unawaited(
dispatcher.dispatchAndComplete(
HostedUiEvent.exchange(
OAuthParameters.fromJson(queryParams),
),
),
);
dispatcher
.dispatch(
HostedUiEvent.exchange(
OAuthParameters.fromJson(queryParams),
),
)
.ignore();
await _respond(
request,
HttpStatus.ok,
Expand All @@ -268,7 +268,7 @@ class HostedUiPlatformImpl extends HostedUiPlatform {
break;
}
} finally {
unawaited(close());
close().ignore();
}
}

Expand Down Expand Up @@ -324,7 +324,7 @@ class HostedUiPlatformImpl extends HostedUiPlatform {
break;
}
} finally {
unawaited(close());
close().ignore();
}
}

Expand Down
Loading

0 comments on commit 802c7c8

Please sign in to comment.