From 0e99647baa78027667c2c4cba873a9c9c9f1bac2 Mon Sep 17 00:00:00 2001 From: Dillon Nys <24740863+dnys1@users.noreply.github.com> Date: Thu, 31 Aug 2023 13:49:32 -0700 Subject: [PATCH] fix(auth): Custom auth with device tracking, no SRP (#3652) The fix in https://github.com/aws-amplify/amplify-flutter/pull/3558 assumed that device SRP would only be performed when password SRP had previously been performed as part of a custom Auth workflow. However, this is not true. In custom auth workflows without password SRP, device SRP can still be required when device tracking is enabled, and in this case it is not a requirement that password SRP have been previously performed. --- infra/lib/stack.ts | 9 +- .../integration_test/custom_auth_test.dart | 523 +++++++++--------- .../state/machines/sign_in_state_machine.dart | 27 +- 3 files changed, 297 insertions(+), 262 deletions(-) diff --git a/infra/lib/stack.ts b/infra/lib/stack.ts index 57b2979344..690db8daa1 100644 --- a/infra/lib/stack.ts +++ b/infra/lib/stack.ts @@ -252,10 +252,17 @@ export class AmplifyFlutterIntegStack extends cdk.Stack { { associateWithWaf, type: "FULL", - environmentName: "custom-auth-device-srp", + environmentName: "custom-auth-device-with-srp", customAuth: "WITH_SRP", deviceTracking: deviceTrackingAlways, }, + { + associateWithWaf, + type: "FULL", + environmentName: "custom-auth-device-without-srp", + customAuth: "WITHOUT_SRP", + deviceTracking: deviceTrackingAlways, + }, { associateWithWaf, type: "FULL", diff --git a/packages/auth/amplify_auth_cognito/example/integration_test/custom_auth_test.dart b/packages/auth/amplify_auth_cognito/example/integration_test/custom_auth_test.dart index 2dca41f313..268bb8e6da 100644 --- a/packages/auth/amplify_auth_cognito/example/integration_test/custom_auth_test.dart +++ b/packages/auth/amplify_auth_cognito/example/integration_test/custom_auth_test.dart @@ -1,12 +1,13 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +// ignore_for_file: invalid_use_of_internal_member + import 'package:amplify_auth_cognito/amplify_auth_cognito.dart'; import 'package:amplify_auth_cognito_dart/src/credentials/device_metadata_repository.dart'; import 'package:amplify_auth_cognito_dart/src/model/cognito_device_secrets.dart'; import 'package:amplify_auth_cognito_dart/src/sdk/cognito_identity_provider.dart' show ChallengeNameType, DeviceRememberedStatusType; -// ignore: invalid_use_of_internal_member import 'package:amplify_auth_cognito_dart/src/state/state.dart'; import 'package:amplify_auth_integration_test/amplify_auth_integration_test.dart'; import 'package:amplify_flutter/amplify_flutter.dart'; @@ -27,81 +28,118 @@ void main() { late String username; late String password; - group('without SRP', () { - const options = SignInOptions( - pluginOptions: CognitoSignInPluginOptions( - authFlowType: AuthenticationFlowType.customAuthWithoutSrp, - ), - ); - - setUp(() async { - await testRunner.configure( - environmentName: 'custom-auth-without-srp', + for (final (testName, environmentName) in [ + ('without SRP', 'custom-auth-without-srp'), + ('without SRP (Device Tracking)', 'custom-auth-device-without-srp'), + ]) { + group(testName, () { + const options = SignInOptions( + pluginOptions: CognitoSignInPluginOptions( + authFlowType: AuthenticationFlowType.customAuthWithoutSrp, + ), ); - username = generateUsername(); - password = generatePassword(); + setUp(() async { + await testRunner.configure( + environmentName: environmentName, + ); - await adminCreateUser( - username, - password, - autoConfirm: true, - verifyAttributes: true, - ); - }); + username = generateUsername(); + password = generatePassword(); - asyncTest( - 'signIn should return data from the auth challenge lambda', - (_) async { - final res = await Amplify.Auth.signIn( - username: username, - options: options, - ); - expect( - res.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, + await adminCreateUser( + username, + password, + autoConfirm: true, + verifyAttributes: true, ); + }); - // additionalInfo key values defined in lambda code - expect(res.nextStep.additionalInfo, { - 'test-key': 'test-value', - 'USERNAME': username, - }); - }, - ); + asyncTest( + 'signIn should return data from the auth challenge lambda', + (_) async { + final res = await Amplify.Auth.signIn( + username: username, + options: options, + ); + expect( + res.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + ); - asyncTest( - 'a correct challenge reply should sign in the user in', - (_) async { - final signInRes = await Amplify.Auth.signIn( - username: username, - options: options, - ); - expect( - signInRes.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, - ); - final res = await Amplify.Auth.confirmSignIn( - confirmationValue: confirmationValue, - ); - expect(res.isSignedIn, isTrue); - }, - ); + // additionalInfo key values defined in lambda code + expect(res.nextStep.additionalInfo, { + 'test-key': 'test-value', + 'USERNAME': username, + }); + }, + ); - asyncTest( - 'an incorrect challenge reply should throw a NotAuthorizedException', - (_) async { - final res = await Amplify.Auth.signIn( - username: username, - options: options, - ); - expect( - res.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, - ); - // Can retry 3 times - for (var retry = 1; retry <= 3; retry++) { - logger.debug('Retry attempt: $retry'); + asyncTest( + 'a correct challenge reply should sign in the user in', + (_) async { + final signInRes = await Amplify.Auth.signIn( + username: username, + options: options, + ); + expect( + signInRes.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + ); + final res = await Amplify.Auth.confirmSignIn( + confirmationValue: confirmationValue, + ); + expect(res.isSignedIn, isTrue); + }, + ); + + asyncTest( + 'an incorrect challenge reply should throw a NotAuthorizedException', + (_) async { + final res = await Amplify.Auth.signIn( + username: username, + options: options, + ); + expect( + res.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + ); + // Can retry 3 times + for (var retry = 1; retry <= 3; retry++) { + logger.debug('Retry attempt: $retry'); + final confirmRes = await Amplify.Auth.confirmSignIn( + confirmationValue: 'wrong', + ); + expect( + confirmRes.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + ); + expect( + confirmRes.nextStep.additionalInfo, + containsPair('errorCode', 'NotAuthorizedException'), + ); + } + await expectLater( + Amplify.Auth.confirmSignIn( + confirmationValue: 'wrong', + ), + throwsA(isA()), + reason: 'After 3 tries, fail completely', + ); + }, + ); + + asyncTest( + 'challenges can be re-attempted on failure', + (_) async { + final res = await Amplify.Auth.signIn( + username: username, + options: options, + ); + expect( + res.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + ); final confirmRes = await Amplify.Auth.confirmSignIn( confirmationValue: 'wrong', ); @@ -113,20 +151,55 @@ void main() { confirmRes.nextStep.additionalInfo, containsPair('errorCode', 'NotAuthorizedException'), ); - } + await expectLater( + Amplify.Auth.confirmSignIn( + confirmationValue: confirmationValue, + ), + completion( + isA().having( + (res) => res.isSignedIn, + 'isSignedIn', + isTrue, + ), + ), + ); + }, + ); + + asyncTest('fails when including a password', (_) async { await expectLater( - Amplify.Auth.confirmSignIn( - confirmationValue: 'wrong', + Amplify.Auth.signIn( + username: username, + password: password, + options: options, ), - throwsA(isA()), - reason: 'After 3 tries, fail completely', + throwsA(isA()), ); - }, - ); + }); - asyncTest( - 'challenges can be re-attempted on failure', - (_) async { + asyncTest( + 'passes client metadata to signIn', + (_) async { + await expectLater( + Amplify.Auth.signIn( + username: username, + options: const SignInOptions( + pluginOptions: CognitoSignInPluginOptions( + authFlowType: AuthenticationFlowType.customAuthWithoutSrp, + clientMetadata: { + 'should-fail': 'true', + }, + ), + ), + ), + throwsA(isA()), + skip: 'Cognito does not pass InitiateAuth client metadata to the ' + 'custom auth triggers: https://docs.aws.amazon.com/cognito-user-identity-pools/latest/APIReference/API_InitiateAuth.html#CognitoUserPools-InitiateAuth-request-ClientMetadata', + ); + }, + ); + + asyncTest('passes client metadata to confirmSignIn', (_) async { final res = await Amplify.Auth.signIn( username: username, options: options, @@ -135,52 +208,11 @@ void main() { res.nextStep.signInStep, AuthSignInStep.confirmSignInWithCustomChallenge, ); - final confirmRes = await Amplify.Auth.confirmSignIn( - confirmationValue: 'wrong', - ); - expect( - confirmRes.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, - ); - expect( - confirmRes.nextStep.additionalInfo, - containsPair('errorCode', 'NotAuthorizedException'), - ); await expectLater( Amplify.Auth.confirmSignIn( confirmationValue: confirmationValue, - ), - completion( - isA().having( - (res) => res.isSignedIn, - 'isSignedIn', - isTrue, - ), - ), - ); - }, - ); - - asyncTest('fails when including a password', (_) async { - await expectLater( - Amplify.Auth.signIn( - username: username, - password: password, - options: options, - ), - throwsA(isA()), - ); - }); - - asyncTest( - 'passes client metadata to signIn', - (_) async { - await expectLater( - Amplify.Auth.signIn( - username: username, - options: const SignInOptions( - pluginOptions: CognitoSignInPluginOptions( - authFlowType: AuthenticationFlowType.customAuthWithoutSrp, + options: const ConfirmSignInOptions( + pluginOptions: CognitoConfirmSignInPluginOptions( clientMetadata: { 'should-fail': 'true', }, @@ -188,73 +220,47 @@ void main() { ), ), throwsA(isA()), - skip: 'Cognito does not pass InitiateAuth client metadata to the ' - 'custom auth triggers: https://docs.aws.amazon.com/cognito-user-identity-pools/latest/APIReference/API_InitiateAuth.html#CognitoUserPools-InitiateAuth-request-ClientMetadata', ); - }, - ); + await expectLater( + Amplify.Auth.confirmSignIn( + confirmationValue: confirmationValue, + ), + throwsA(isA()), + reason: 'Authorization failures are not retryable', + ); + }); - asyncTest('passes client metadata to confirmSignIn', (_) async { - final res = await Amplify.Auth.signIn( - username: username, - options: options, - ); - expect( - res.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, - ); - await expectLater( - Amplify.Auth.confirmSignIn( - confirmationValue: confirmationValue, - options: const ConfirmSignInOptions( - pluginOptions: CognitoConfirmSignInPluginOptions( - clientMetadata: { - 'should-fail': 'true', - }, - ), + asyncTest('works with deprecated auth flow', (_) async { + const options = SignInOptions( + pluginOptions: CognitoSignInPluginOptions( + // ignore: deprecated_member_use + authFlowType: AuthenticationFlowType.customAuth, ), - ), - throwsA(isA()), - ); - await expectLater( - Amplify.Auth.confirmSignIn( + ); + final signInRes = await Amplify.Auth.signIn( + username: username, + options: options, + ); + expect( + signInRes.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + reason: 'should use non-SRP flow based on exclusion of password', + ); + final res = await Amplify.Auth.confirmSignIn( confirmationValue: confirmationValue, - ), - throwsA(isA()), - reason: 'Authorization failures are not retryable', - ); - }); - - asyncTest('works with deprecated auth flow', (_) async { - const options = SignInOptions( - pluginOptions: CognitoSignInPluginOptions( - // ignore: deprecated_member_use - authFlowType: AuthenticationFlowType.customAuth, - ), - ); - final signInRes = await Amplify.Auth.signIn( - username: username, - options: options, - ); - expect( - signInRes.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, - reason: 'should use non-SRP flow based on exclusion of password', - ); - final res = await Amplify.Auth.confirmSignIn( - confirmationValue: confirmationValue, - ); - expect( - res.isSignedIn, - isTrue, - reason: 'should use non-SRP flow based on exclusion of password', - ); + ); + expect( + res.isSignedIn, + isTrue, + reason: 'should use non-SRP flow based on exclusion of password', + ); + }); }); - }); + } for (final (testName, environmentName) in [ ('with SRP', 'custom-auth-with-srp'), - ('with Device SRP', 'custom-auth-device-srp'), + ('with SRP (Device Tracking)', 'custom-auth-device-with-srp'), ]) { group(testName, () { const options = SignInOptions( @@ -446,82 +452,105 @@ void main() { }); } - group('with Device SRP', () { - const options = SignInOptions( - pluginOptions: CognitoSignInPluginOptions( - authFlowType: AuthenticationFlowType.customAuthWithSrp, + group('with Device Tracking', () { + for (final (environmentName, authFlowType) in [ + // See: https://github.com/aws-amplify/amplify-flutter/issues/3541#issuecomment-1675384073 + ( + 'custom-auth-device-with-srp', + AuthenticationFlowType.customAuthWithSrp + ), + // See: https://github.com/aws-amplify/amplify-flutter/issues/3596#issue-1862400577 + ( + 'custom-auth-device-without-srp', + AuthenticationFlowType.customAuthWithoutSrp ), - ); + ]) { + group(environmentName, () { + setUp(() async { + await testRunner.configure( + environmentName: environmentName, + ); - setUp(() async { - await testRunner.configure( - environmentName: 'custom-auth-device-srp', - ); + username = generateUsername(); + password = generatePassword(); - username = generateUsername(); - password = generatePassword(); + await adminCreateUser( + username, + password, + autoConfirm: true, + verifyAttributes: true, + ); + }); - await adminCreateUser( - username, - password, - autoConfirm: true, - verifyAttributes: true, - ); - }); + Future signIn() async { + final signInRes = await switch (authFlowType) { + AuthenticationFlowType.customAuthWithSrp => Amplify.Auth.signIn( + username: username, + password: password, + options: const SignInOptions( + pluginOptions: CognitoSignInPluginOptions( + authFlowType: AuthenticationFlowType.customAuthWithSrp, + ), + ), + ), + AuthenticationFlowType.customAuthWithoutSrp => + Amplify.Auth.signIn( + username: username, + options: const SignInOptions( + pluginOptions: CognitoSignInPluginOptions( + authFlowType: AuthenticationFlowType.customAuthWithoutSrp, + ), + ), + ), + _ => throw StateError('unreachable'), + }; + expect( + signInRes.nextStep.signInStep, + AuthSignInStep.confirmSignInWithCustomChallenge, + ); + final res = await Amplify.Auth.confirmSignIn( + confirmationValue: confirmationValue, + ); + expect(res.isSignedIn, isTrue); + } - Future signIn() async { - final signInRes = await Amplify.Auth.signIn( - username: username, - password: password, - options: options, - ); - expect( - signInRes.nextStep.signInStep, - AuthSignInStep.confirmSignInWithCustomChallenge, - ); - final res = await Amplify.Auth.confirmSignIn( - confirmationValue: confirmationValue, - ); - expect(res.isSignedIn, isTrue); - } + test('automatically performs device SRP', () async { + // On initial sign-in, the device will be untracked and unremembered. + await signIn(); - // See: https://github.com/aws-amplify/amplify-flutter/issues/3541#issuecomment-1675384073 - test('automatically performs device SRP', () async { - // On initial sign-in, the device will be untracked and unremembered. - await signIn(); - - final deviceRepo = - cognitoPlugin.stateMachine.getOrCreate(); - await expectLater( - deviceRepo.get(username), - completion( - // ignore: invalid_use_of_internal_member - isA().having( - (secrets) => secrets.deviceStatus, - 'deviceStatus', - DeviceRememberedStatusType.remembered, - ), - ), - reason: 'Device should be remembered due to backend config', - ); + final deviceRepo = cognitoPlugin.stateMachine + .getOrCreate(); + await expectLater( + deviceRepo.get(username), + completion( + isA().having( + (secrets) => secrets.deviceStatus, + 'deviceStatus', + DeviceRememberedStatusType.remembered, + ), + ), + reason: 'Device should be remembered due to backend config', + ); - await signOutUser(); + await signOutUser(); - expect( - cognitoPlugin.stateMachine.stream, - emitsThrough( - isA().having( - (state) => state.challengeName, - 'challengeName', - ChallengeNameType.devicePasswordVerifier, - ), - ), - reason: 'Devices which are tracked and remembered automatically ' - 'trigger device SRP at the end of a custom auth flow', - ); + expect( + cognitoPlugin.stateMachine.stream, + emitsThrough( + isA().having( + (state) => state.challengeName, + 'challengeName', + ChallengeNameType.devicePasswordVerifier, + ), + ), + reason: 'Devices which are tracked and remembered automatically ' + 'trigger device SRP at the end of a custom auth flow', + ); - await signIn(); - }); + await signIn(); + }); + }); + } }); }); } diff --git a/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/sign_in_state_machine.dart b/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/sign_in_state_machine.dart index 9e30a04000..88028bcc10 100644 --- a/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/sign_in_state_machine.dart +++ b/packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/sign_in_state_machine.dart @@ -133,6 +133,13 @@ final class SignInStateMachine return worker; }); + /// Creates the initial SRP public/private values used in SRP handshakes. + Future _initSrp() async { + final worker = await initWorker; + worker.add(SrpInitMessage()); + return worker.stream.first; + } + /// The SRP password verifier worker. Future get passwordVerifierWorker => _passwordVerifierWorkerMemoizer.runOnce(() async { @@ -389,10 +396,10 @@ final class SignInStateMachine /// Creates the device SRP auth request to initiate the device SRP flow. @protected Future createDeviceSrpAuthRequest() async { - final initResult = _initResult; - if (initResult == null) { - throw StateError('Must call init first'); - } + // Device SRP auth does not require SRP auth to have been performed already. + // In custom auth flows which do not begin with SRP auth but where device + // tracking is enabled, Cognito will still perform a device SRP challenge. + _initResult ??= await _initSrp(); return RespondToAuthChallengeRequest.build((b) { b ..clientId = config.appClientId @@ -402,7 +409,7 @@ final class SignInStateMachine CognitoConstants.challengeParamDeviceKey: _user.deviceSecrets!.deviceKey!, CognitoConstants.challengeParamSrpA: - initResult.publicA.toRadixString(16), + _initResult!.publicA.toRadixString(16), }); }); } @@ -412,11 +419,6 @@ final class SignInStateMachine Future createDevicePasswordVerifierRequest( BuiltMap challengeParameters, ) async { - final password = parameters.password; - if (password == null || password.isEmpty) { - throw const AuthValidationException('No password given'); - } - final worker = await devicePasswordVerifierWorker; final workerMessage = SrpDevicePasswordVerifierMessage((b) { b @@ -493,10 +495,7 @@ final class SignInStateMachine /// Initiates an SRP flow. @protected Future initiateSrpAuth(SignInInitiate event) async { - final worker = await initWorker; - worker.add(SrpInitMessage()); - _initResult = await worker.stream.first; - + _initResult = await _initSrp(); return InitiateAuthRequest.build((b) { b ..authFlow = AuthFlowType.userSrpAuth