diff --git a/client/lib/enterprise/screens/mfa/openid_mfa_waiting_screen.dart b/client/lib/enterprise/screens/mfa/openid_mfa_waiting_screen.dart index e34881b8..320b6f21 100644 --- a/client/lib/enterprise/screens/mfa/openid_mfa_waiting_screen.dart +++ b/client/lib/enterprise/screens/mfa/openid_mfa_waiting_screen.dart @@ -78,7 +78,7 @@ class OpenIdMfaWaitingScreen extends HookConsumerWidget { .catchError((error) { if (isGone()) return; host.reportFailure( - message: ErrorHandler.getHumanReadableError(error), + message: ErrorHandler.getOpenIdMfaError(error), logMessage: "OpenID MFA polling error!", error: error, ); diff --git a/client/lib/open/api.dart b/client/lib/open/api.dart index b871e43d..7fe3424a 100644 --- a/client/lib/open/api.dart +++ b/client/lib/open/api.dart @@ -48,7 +48,15 @@ class MfaMethodNotAvailableException implements Exception { 'Requested MFA method is not available on the account: ${method.toReadableString()}'; } -enum MfaRequestFailure { network, policyDenied, attemptLimit, notReady, server } +enum MfaRequestFailure { + network, + policyDenied, + attemptLimit, + sessionExpired, + staleAttempt, + notReady, + server, +} class MfaRequestException implements Exception { final String operation; @@ -69,10 +77,38 @@ class MfaRequestException implements Exception { : '$operation failed. Status: $statusCode'; } +MfaRequestFailure? _mfaResponseFailure( + DioException error, { + bool emptyForbiddenIsAttemptLimit = false, +}) { + final response = error.response; + final data = response?.data; + final message = data is Map ? data['error'] : null; + + if (response?.statusCode == 400 && message == 'login session not found') { + return MfaRequestFailure.sessionExpired; + } + if (response?.statusCode == 401 && message == 'invalid token') { + return MfaRequestFailure.sessionExpired; + } + if (response?.statusCode == 400 && message == 'stale MFA attempt') { + return MfaRequestFailure.staleAttempt; + } + if (response?.statusCode == 403 && + (message == + 'Too many failed MFA attempts. Please try connecting again.' || + (emptyForbiddenIsAttemptLimit && (data == null || data == '')))) { + return MfaRequestFailure.attemptLimit; + } + return null; +} + MfaRequestException _mfaRequestException( String operation, DioException error, { Map statusFailures = const {}, + bool classifyResponse = false, + bool emptyForbiddenIsAttemptLimit = false, }) { final statusCode = error.response?.statusCode; final networkError = @@ -86,7 +122,15 @@ MfaRequestException _mfaRequestException( statusCode: statusCode, failure: networkError ? MfaRequestFailure.network - : statusFailures[statusCode] ?? MfaRequestFailure.server, + : (classifyResponse + ? _mfaResponseFailure( + error, + emptyForbiddenIsAttemptLimit: + emptyForbiddenIsAttemptLimit, + ) + : null) ?? + statusFailures[statusCode] ?? + MfaRequestFailure.server, ); } @@ -404,10 +448,10 @@ class ProxyApi { if (response.statusCode == 428) return null; return FinishMfaResponse.fromJson(response.data); } on DioException catch (e) { - if (e.response?.statusCode == 401) { + if (e.response?.statusCode == 401 && _mfaResponseFailure(e) == null) { throw const MfaCodeRejectedException(); } - throw _mfaRequestException('MFA finish', e); + throw _mfaRequestException('MFA finish', e, classifyResponse: true); } catch (e) { talker.error('Invalid MFA finish response: ${e.runtimeType}'); throw const FormatException('Invalid MFA finish response'); @@ -429,13 +473,14 @@ class ProxyApi { final response = await _dio.postUri(endpoint, data: data.toJson()); return wire.MfaFlowStepFinishResponse.fromJson(response.data); } on DioException catch (e) { - if (e.response?.statusCode == 401) { + if (e.response?.statusCode == 401 && _mfaResponseFailure(e) == null) { throw const MfaCodeRejectedException(); } throw _mfaRequestException( 'MFA flow step finish', e, - statusFailures: const {403: MfaRequestFailure.attemptLimit}, + classifyResponse: true, + emptyForbiddenIsAttemptLimit: true, ); } catch (e) { talker.error('Invalid MFA flow step finish response: ${e.runtimeType}'); diff --git a/client/lib/utils/error_handler.dart b/client/lib/utils/error_handler.dart index 97cf7bbe..451c9108 100644 --- a/client/lib/utils/error_handler.dart +++ b/client/lib/utils/error_handler.dart @@ -35,6 +35,8 @@ class ErrorHandler { "This device does not meet the location's security requirements.", MfaRequestFailure.attemptLimit => "Too many failed attempts. Start connecting again.", + MfaRequestFailure.sessionExpired => _statusMessage(e.statusCode), + MfaRequestFailure.staleAttempt => _statusMessage(e.statusCode), MfaRequestFailure.notReady => "Verification can't start right now. Try again in a moment.", MfaRequestFailure.server => _statusMessage(e.statusCode), @@ -61,6 +63,28 @@ class ErrorHandler { return s; } + static String getOpenIdMfaError(Object e) { + if (e is DioException && e.response == null) { + return getHumanReadableError(e); + } + if (e is! MfaRequestException) { + return "Authentication failed. Please try again."; + } + return switch (e.failure) { + MfaRequestFailure.network => _connectionErrorMessage, + MfaRequestFailure.policyDenied => + "This device does not meet the location's security requirements.", + MfaRequestFailure.attemptLimit => + "Too many failed MFA attempts. Please try connecting again.", + MfaRequestFailure.sessionExpired => "Session expired. Please try again.", + MfaRequestFailure.staleAttempt => + "Authentication request could not be started. Please try again.", + MfaRequestFailure.notReady => + "Verification can't start right now. Try again in a moment.", + MfaRequestFailure.server => "Authentication failed. Please try again.", + }; + } + static const _connectionErrorMessage = "Unable to connect to the server. Please check your internet connection."; diff --git a/client/test/enterprise/openid_mfa_waiting_screen_test.dart b/client/test/enterprise/openid_mfa_waiting_screen_test.dart index e1744bff..5a5b4c67 100644 --- a/client/test/enterprise/openid_mfa_waiting_screen_test.dart +++ b/client/test/enterprise/openid_mfa_waiting_screen_test.dart @@ -9,15 +9,19 @@ import 'package:mobile/data/mfa/mfa_flow.dart'; import 'package:mobile/data/mfa/mfa_transport.dart'; import 'package:mobile/enterprise/postures.dart'; import 'package:mobile/enterprise/screens/mfa/openid_mfa_waiting_screen.dart'; +import 'package:mobile/open/api.dart'; import 'package:mobile/open/screens/mfa/mfa_step_chrome.dart'; class _PollingTransport implements MfaTransport { final Queue results; + final Object? finishError; final stepAttemptIds = []; final credentials = []; - _PollingTransport(Iterable results) - : results = Queue.of(results); + _PollingTransport( + Iterable results, { + this.finishError, + }) : results = Queue.of(results); @override Future start({ @@ -42,6 +46,7 @@ class _PollingTransport implements MfaTransport { }) async { stepAttemptIds.add(stepAttemptId); credentials.add(credential); + if (finishError != null) throw finishError!; return results.removeFirst(); } } @@ -50,6 +55,7 @@ class _Host implements MfaStepHost { @override final MfaFlowController controller; final progress = Completer(); + final failure = Completer(); _Host(this.controller); @@ -64,7 +70,7 @@ class _Host implements MfaStepHost { String? logMessage, Object? error, }) { - if (!progress.isCompleted) progress.completeError(StateError(message)); + if (!failure.isCompleted) failure.complete(message); } @override @@ -75,6 +81,67 @@ class _Host implements MfaStepHost { } void main() { + testWidgets('reports a safe OIDC failure through the host', (tester) async { + final transport = _PollingTransport( + [], + finishError: MfaRequestException( + 'MFA flow step finish', + statusCode: 400, + failure: MfaRequestFailure.sessionExpired, + ), + ); + final controller = MfaFlowController( + transport: transport, + plan: [MfaMethod.openid], + devicePubkey: 'device-pubkey', + networkId: 11, + ); + await controller.startStep(); + final host = _Host(controller); + + await tester.pumpWidget( + ProviderScope( + child: MaterialApp(home: OpenIdMfaWaitingScreen(host: host)), + ), + ); + await tester.pumpAndSettle(); + + expect(await host.failure.future, 'Session expired. Please try again.'); + }); + + testWidgets('hides unknown OIDC response details from the host', ( + tester, + ) async { + final transport = _PollingTransport( + [], + finishError: MfaRequestException( + 'MFA flow step finish', + statusCode: 400, + failure: MfaRequestFailure.server, + ), + ); + final controller = MfaFlowController( + transport: transport, + plan: [MfaMethod.openid], + devicePubkey: 'device-pubkey', + networkId: 11, + ); + await controller.startStep(); + final host = _Host(controller); + + await tester.pumpWidget( + ProviderScope( + child: MaterialApp(home: OpenIdMfaWaitingScreen(host: host)), + ), + ); + await tester.pumpAndSettle(); + + expect( + await host.failure.future, + 'Authentication failed. Please try again.', + ); + }); + testWidgets('polls without a proof until the OIDC attempt completes', ( tester, ) async { diff --git a/client/test/mfa/mfa_code_screen_finish_error_test.dart b/client/test/mfa/mfa_code_screen_finish_error_test.dart new file mode 100644 index 00000000..6c223722 --- /dev/null +++ b/client/test/mfa/mfa_code_screen_finish_error_test.dart @@ -0,0 +1,130 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:material_ui/material_ui.dart'; +import 'package:mobile/data/db/enums.dart'; +import 'package:mobile/data/mfa/mfa_flow.dart'; +import 'package:mobile/data/mfa/mfa_transport.dart'; +import 'package:mobile/enterprise/postures.dart'; +import 'package:mobile/open/api.dart'; +import 'package:mobile/open/screens/mfa/mfa_code_screen.dart'; +import 'package:mobile/open/screens/mfa/mfa_step_chrome.dart'; +import 'package:mobile/open/widgets/toaster/toast_manager.dart'; + +class _FailingTransport implements MfaTransport { + final MfaRequestFailure failure; + + const _FailingTransport(this.failure); + + @override + Future start({ + required String devicePubkey, + required int networkId, + required List plan, + DevicePostureData? postureData, + }) async => const MfaSessionStart( + token: 'flow-token', + firstStep: MfaStepChallenge(stepAttemptId: 'attempt-1'), + ); + + @override + Future startStep(String token, MfaMethod method) async => + const MfaStepChallenge(stepAttemptId: 'attempt-1'); + + @override + Future finish({ + required String token, + required String? stepAttemptId, + required MfaCredential? credential, + }) async { + throw MfaRequestException( + 'MFA flow step finish', + statusCode: 403, + failure: failure, + ); + } +} + +class _TestHost implements MfaStepHost { + @override + final MfaFlowController controller; + String? failureMessage; + + _TestHost(MfaRequestFailure failure) + : controller = MfaFlowController( + transport: _FailingTransport(failure), + plan: const [MfaMethod.totp], + devicePubkey: 'device-key', + networkId: 11, + ); + + @override + void reportFailure({ + required String message, + String? logMessage, + Object? error, + }) { + failureMessage = message; + } + + @override + void reportProgress(MfaStepProgress progress) {} + + @override + void abort() {} +} + +void main() { + Future<(String?, ToastData?)> submitCode( + WidgetTester tester, + MfaRequestFailure failure, + ) async { + final host = _TestHost(failure); + final container = ProviderContainer(); + addTearDown(container.dispose); + await host.controller.startStep(); + await tester.pumpWidget( + UncontrolledProviderScope( + container: container, + child: MaterialApp( + home: MfaCodeScreen( + host: host, + title: 'Verification', + description: 'Enter your code.', + fieldLabel: 'Code', + logLabel: 'MFA', + ), + ), + ), + ); + await tester.enterText(find.byType(TextField), '123456'); + await tester.tap(find.text('Submit')); + await tester.pumpAndSettle(); + final toast = container.read(toastManagerProvider); + container.read(toastManagerProvider.notifier).remove(); + return (host.failureMessage, toast); + } + + testWidgets('an exhausted finish is reported with attempt-limit copy', ( + tester, + ) async { + final (failureMessage, toast) = await submitCode( + tester, + MfaRequestFailure.attemptLimit, + ); + + expect(failureMessage, 'Too many failed attempts. Start connecting again.'); + expect(toast, isNull); + }); + + testWidgets('an unrelated finish 403 remains a generic screen error', ( + tester, + ) async { + final (failureMessage, toast) = await submitCode( + tester, + MfaRequestFailure.server, + ); + + expect(failureMessage, isNull); + expect(toast?.message, 'Access forbidden.'); + }); +} diff --git a/client/test/mfa/mfa_flow_api_test.dart b/client/test/mfa/mfa_flow_api_test.dart index bc27843a..ea491803 100644 --- a/client/test/mfa/mfa_flow_api_test.dart +++ b/client/test/mfa/mfa_flow_api_test.dart @@ -295,10 +295,11 @@ void main() { Future expectFailure( int status, Future Function() call, - MfaRequestFailure failure, - ) async { + MfaRequestFailure failure, { + String responseBody = '{"error":"server text"}', + }) async { adapter.statusCode = status; - adapter.responseBody = '{"error":"server text"}'; + adapter.responseBody = responseBody; await expectLater(call(), failsWith(failure)); } @@ -350,6 +351,219 @@ void main() { ), ), MfaRequestFailure.attemptLimit, + responseBody: + '{"error":"Too many failed MFA attempts. Please try connecting again."}', + ); + }); + + test( + 'legacy finish classifies an expired session without retaining text', + () async { + adapter.statusCode = 400; + adapter.responseBody = '{"error":"login session not found"}'; + + await expectLater( + api.finishMfa( + url, + const FinishMfaRequest(token: 'legacy-token'), + ), + throwsA( + isA() + .having( + (error) => error.failure.name, + 'failure', + 'sessionExpired', + ) + .having( + (error) => error.toString(), + 'safe error text', + isNot(contains('login session not found')), + ), + ), + ); + }, + ); + + test( + 'flow finish classifies stale attempts without retaining text', + () async { + adapter.statusCode = 400; + adapter.responseBody = '{"error":"stale MFA attempt"}'; + + await expectLater( + api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + throwsA( + isA() + .having( + (error) => error.failure.name, + 'failure', + 'staleAttempt', + ) + .having( + (error) => error.toString(), + 'safe error text', + isNot(contains('stale MFA attempt')), + ), + ), + ); + }, + ); + + test('finish classifies only the known attempt-limit reason as such', () async { + await expectFailure( + 403, + () => api.finishMfa( + url, + const FinishMfaRequest(token: 'legacy-token'), + ), + MfaRequestFailure.attemptLimit, + responseBody: + '{"error":"Too many failed MFA attempts. Please try connecting again."}', + ); + await expectFailure( + 403, + () => api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + MfaRequestFailure.attemptLimit, + responseBody: + '{"error":"Too many failed MFA attempts. Please try connecting again."}', + ); + }); + + test('an empty flow-finish 403 is treated as an attempt limit', () async { + await expectFailure( + 403, + () => api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + MfaRequestFailure.attemptLimit, + responseBody: '', + ); + }); + + test('an explicit unrelated flow-finish 403 stays generic', () async { + await expectFailure( + 403, + () => api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + MfaRequestFailure.server, + responseBody: '{"error":"posture rejected"}', + ); + }); + + test('invalid tokens on 401 are session-expired failures', () async { + adapter.statusCode = 401; + adapter.responseBody = '{"error":"invalid token"}'; + + final legacyFinish = api.finishMfa( + url, + const FinishMfaRequest(token: 'legacy-token'), + ); + await expectLater( + legacyFinish, + throwsA( + isA().having( + (error) => error.failure.name, + 'failure', + 'sessionExpired', + ), + ), + ); + + final flowFinish = api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ); + await expectLater( + flowFinish, + throwsA( + isA().having( + (error) => error.failure.name, + 'failure', + 'sessionExpired', + ), + ), + ); + }); + + test('known reasons on unexpected statuses stay server failures', () async { + await expectFailure( + 500, + () => api.finishMfa( + url, + const FinishMfaRequest(token: 'legacy-token'), + ), + MfaRequestFailure.server, + responseBody: '{"error":"login session not found"}', + ); + await expectFailure( + 500, + () => api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + MfaRequestFailure.server, + responseBody: '{"error":"stale MFA attempt"}', + ); + await expectFailure( + 500, + () => api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + MfaRequestFailure.server, + responseBody: + '{"error":"Too many failed MFA attempts. Please try connecting again."}', + ); + await expectFailure( + 400, + () => api.finishMfaFlow( + url, + const flow.MfaFlowStepFinishRequest( + token: 'flow-token', + stepAttemptId: 'attempt-1', + ), + ), + MfaRequestFailure.server, + responseBody: '{"error":"invalid token"}', + ); + }); + + test('known finish reasons do not classify start errors', () async { + await expectFailure( + 400, + () => api.startMfaFlow(url, flowStart), + MfaRequestFailure.server, + responseBody: '{"error":"stale MFA attempt"}', ); }); diff --git a/client/test/utils/error_handler_test.dart b/client/test/utils/error_handler_test.dart index d06a7446..81297dbc 100644 --- a/client/test/utils/error_handler_test.dart +++ b/client/test/utils/error_handler_test.dart @@ -25,12 +25,60 @@ void main() { message(MfaRequestFailure.attemptLimit, statusCode: 403), 'Too many failed attempts. Start connecting again.', ); + expect( + message(MfaRequestFailure.sessionExpired, statusCode: 400), + 'Server returned an error: 400', + ); + expect( + message(MfaRequestFailure.staleAttempt, statusCode: 400), + 'Server returned an error: 400', + ); expect( message(MfaRequestFailure.notReady, statusCode: 428), "Verification can't start right now. Try again in a moment.", ); }); + test('OpenID failures use fixed desktop-style messages', () { + String openId(MfaRequestFailure failure, {int? statusCode}) => + ErrorHandler.getOpenIdMfaError( + MfaRequestException( + 'MFA flow step finish', + statusCode: statusCode, + failure: failure, + ), + ); + + expect( + openId(MfaRequestFailure.sessionExpired), + 'Session expired. Please try again.', + ); + expect( + openId(MfaRequestFailure.staleAttempt), + 'Authentication request could not be started. Please try again.', + ); + expect( + openId(MfaRequestFailure.attemptLimit), + 'Too many failed MFA attempts. Please try connecting again.', + ); + expect( + openId(MfaRequestFailure.network), + 'Unable to connect to the server. Please check your internet connection.', + ); + expect( + openId(MfaRequestFailure.notReady), + "Verification can't start right now. Try again in a moment.", + ); + expect( + openId(MfaRequestFailure.server, statusCode: 400), + 'Authentication failed. Please try again.', + ); + expect( + ErrorHandler.getOpenIdMfaError(StateError('raw response text')), + 'Authentication failed. Please try again.', + ); + }); + test('other MFA server failures read like other HTTP errors', () { expect( message(MfaRequestFailure.server, statusCode: 503),