From 554aac6af354c6ba0592087d7352eec8e4a6f046 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 19 Dec 2024 16:29:41 -0500 Subject: [PATCH] store: Handle invalid API key on register-queue The method loadPerAccount has two call sites, i.e. places where we send register-queue requests: 1. _reloadPerAccount through [UpdateMachine._handlePollError] (e.g.: expired event queue) 2. perAccount through [PerAccountStoreWidget] (e.g.: loading for the first time) Both sites already expect [AccountNotFoundException] by assuming that the `loadPerAccount` fail is irrecoverable and is handled elsewhere. This partly addresses #890 by handling authentication errors for register-queue. Fixes: #737 Signed-off-by: Zixuan James Li --- assets/l10n/app_en.arb | 7 ++ lib/generated/l10n/zulip_localizations.dart | 6 ++ .../l10n/zulip_localizations_ar.dart | 5 ++ .../l10n/zulip_localizations_en.dart | 5 ++ .../l10n/zulip_localizations_ja.dart | 5 ++ .../l10n/zulip_localizations_nb.dart | 5 ++ .../l10n/zulip_localizations_pl.dart | 5 ++ .../l10n/zulip_localizations_ru.dart | 5 ++ .../l10n/zulip_localizations_sk.dart | 5 ++ lib/model/store.dart | 47 +++++++++-- test/model/store_test.dart | 33 +++++++- test/model/test_store.dart | 4 + test/widgets/app_test.dart | 80 +++++++++++++++++++ 13 files changed, 205 insertions(+), 7 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index f83ab27199..118ab83c70 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -523,6 +523,13 @@ "@topicValidationErrorMandatoryButEmpty": { "description": "Topic validation error when topic is required but was empty." }, + "errorInvalidApiKeyMessage": "Your account at {url} could not be authenticated. Please try logging in again or use another account.", + "@errorInvalidApiKeyMessage": { + "description": "Error message in the dialog for invalid API key.", + "placeholders": { + "url": {"type": "String", "example": "http://chat.example.com/"} + } + }, "errorInvalidResponse": "The server sent an invalid response", "@errorInvalidResponse": { "description": "Error message when an API call returned an invalid response." diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index 3946112973..9579683908 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -801,6 +801,12 @@ abstract class ZulipLocalizations { /// **'Topics are required in this organization.'** String get topicValidationErrorMandatoryButEmpty; + /// Error message in the dialog for invalid API key. + /// + /// In en, this message translates to: + /// **'Your account at {url} could not be authenticated. Please try logging in again or use another account.'** + String errorInvalidApiKeyMessage(String url); + /// Error message when an API call returned an invalid response. /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index 07983fe188..71bf06d8ce 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index b1f5cdaf8f..7a33e33567 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index 258e51dce0..137883e5e9 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index 95650fe317..3dec7d9b5a 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'The server sent an invalid response'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index c8151c8daf..83a777bfc4 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Wątki są wymagane przez tę organizację.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index 876f2ad89a..827aaf0155 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Темы обязательны в этой организации.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'Получен недопустимый ответ сервера'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index d59c8456c0..38a3f8a240 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -404,6 +404,11 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.'; + @override + String errorInvalidApiKeyMessage(String url) { + return 'Your account at $url could not be authenticated. Please try logging in again or use another account.'; + } + @override String get errorInvalidResponse => 'Server poslal nesprávnu odpoveď'; diff --git a/lib/model/store.dart b/lib/model/store.dart index 002a7c1ce4..611494cc8f 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -19,6 +19,7 @@ import '../api/backoff.dart'; import '../api/route/realm.dart'; import '../log.dart'; import '../notifications/receive.dart'; +import 'actions.dart'; import 'autocomplete.dart'; import 'database.dart'; import 'emoji.dart'; @@ -149,7 +150,34 @@ abstract class GlobalStore extends ChangeNotifier { /// and/or [perAccountSync]. Future loadPerAccount(int accountId) async { assert(_accounts.containsKey(accountId)); - final store = await doLoadPerAccount(accountId); + final PerAccountStore store; + try { + store = await doLoadPerAccount(accountId); + } catch (e) { + switch (e) { + case HttpException(httpStatus: 401): + // The API key is invalid and the store can never be loaded + // unless the user retries manually. + final account = getAccount(accountId); + if (account == null) { + // The account was logged out during `await doLoadPerAccount`. + // Here, that seems possible only by the user's own action; + // the logout can't have been done programmatically. + // Even if it were, it would have come with its own UI feedback. + // Anyway, skip showing feedback, to not be confusing or repetitive. + throw AccountNotFoundException(); + } + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + reportErrorToUserModally( + zulipLocalizations.errorCouldNotConnectTitle, + message: zulipLocalizations.errorInvalidApiKeyMessage( + account.realmUrl.toString())); + await logOutAccount(this, accountId); + throw AccountNotFoundException(); + default: + rethrow; + } + } if (!_accounts.containsKey(accountId)) { // TODO(#1354): handle this earlier // [removeAccount] was called during [doLoadPerAccount]. @@ -914,12 +942,19 @@ class UpdateMachine { try { return await registerQueue(connection); } catch (e, s) { - assert(debugLog('Error fetching initial snapshot: $e')); - // Print stack trace in its own log entry; log entries are truncated - // at 1 kiB (at least on Android), and stack can be longer than that. - assert(debugLog('Stack:\n$s')); - assert(debugLog('Backing off, then will retry…')); // TODO(#890): tell user if initial-fetch errors persist, or look non-transient + switch (e) { + case HttpException(httpStatus: 401): + // We cannot recover from this error through retrying. + // Leave it to [GlobalStore.loadPerAccount]. + rethrow; + default: + assert(debugLog('Error fetching initial snapshot: $e')); + // Print stack trace in its own log entry; log entries are truncated + // at 1 kiB (at least on Android), and stack can be longer than that. + assert(debugLog('Stack:\n$s')); + } + assert(debugLog('Backing off, then will retry…')); await (backoffMachine ??= BackoffMachine()).wait(); assert(debugLog('… Backoff wait complete, retrying initial fetch.')); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 7d552a3c00..4ff5031ee5 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -121,6 +121,29 @@ void main() { check(completers(1)).length.equals(1); }); + test('GlobalStore.perAccount loading fails with HTTP status code 401', () => awaitFakeAsync((async) async { + final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]); + final future = globalStore.perAccount(eg.selfAccount.id); + + globalStore.completers[eg.selfAccount.id]! + .single.completeError(eg.apiExceptionUnauthorized()); + await check(future).throws(); + })); + + test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async { + final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]); + final future = globalStore.perAccount(eg.selfAccount.id); + + await logOutAccount(globalStore, eg.selfAccount.id); + check(globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + globalStore.completers[eg.selfAccount.id]! + .single.completeError(eg.apiExceptionUnauthorized()); + await check(future).throws(); + check(globalStore.takeDoRemoveAccountCalls()).isEmpty(); + })); + // TODO test insertAccount group('GlobalStore.updateAccount', () { @@ -997,7 +1020,7 @@ void main() { } void checkReloadFailure({ - required Future Function() completeLoading, + required FutureOr Function() completeLoading, }) { awaitFakeAsync((async) async { await prepareReload(async); @@ -1027,6 +1050,14 @@ void main() { test('user logged out before new store is loaded', () => awaitFakeAsync((async) async { checkReloadFailure(completeLoading: logOutAndCompleteWithNewStore); })); + + void completeWithApiExceptionUnauthorized() { + completers().single.completeError(eg.apiExceptionUnauthorized()); + } + + test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async { + checkReloadFailure(completeLoading: completeWithApiExceptionUnauthorized); + })); }); group('UpdateMachine.registerNotificationToken', () { diff --git a/test/model/test_store.dart b/test/model/test_store.dart index b6887cbed7..534a6003b5 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -129,6 +129,7 @@ class TestGlobalStore extends GlobalStore { static const Duration removeAccountDuration = Duration(milliseconds: 1); Duration? loadPerAccountDuration; + Object? loadPerAccountException; /// Consume the log of calls made to [doRemoveAccount]. List takeDoRemoveAccountCalls() { @@ -150,6 +151,9 @@ class TestGlobalStore extends GlobalStore { if (loadPerAccountDuration != null) { await Future.delayed(loadPerAccountDuration!); } + if (loadPerAccountException != null) { + throw loadPerAccountException!; + } final initialSnapshot = _initialSnapshots[accountId]!; final store = PerAccountStore.fromInitialSnapshot( globalStore: this, diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index d0848622cf..9a39c3ec5a 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -61,15 +61,18 @@ void main() { group('_PreventEmptyStack', () { late List> pushedRoutes; late List> removedRoutes; + late List> poppedRoutes; Future prepare(WidgetTester tester) async { addTearDown(testBinding.reset); pushedRoutes = []; removedRoutes = []; + poppedRoutes = []; final testNavObserver = TestNavigatorObserver(); testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); + testNavObserver.onPopped = (route, prevRoute) => poppedRoutes.add(route); await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); await tester.pump(); // start to load account @@ -91,6 +94,83 @@ void main() { check(removedRoutes).single.isA().page.isA(); check(pushedRoutes).single.isA().page.isA(); }); + + testWidgets('push route when popping last route on stack', (tester) async { + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + + testBinding.globalStore.loadPerAccountDuration = Duration.zero; + testBinding.globalStore.loadPerAccountException = eg.apiExceptionUnauthorized(); + await prepare(tester); + // The navigator stack should contain only a home page route. + + await tester.pump(Duration.zero); // got the error + await tester.pump(TestGlobalStore.removeAccountDuration); + // The navigator stack should contain only a dialog route. + // The home page route was removed because of account logout. + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + check(removedRoutes).single.isA().page.isA(); + check(poppedRoutes).isEmpty(); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Could not connect', + expectedMessage: + 'Your account at ${eg.selfAccount.realmUrl} could not be authenticated.' + ' Please try logging in again or use another account.'))); + // The navigator stack should contain only a choose-account page route. + // After the error dialog is dismissed, it becomes empty, + // so a choose-account page route should be pushed. + check(poppedRoutes).single.isA>(); + check(pushedRoutes).single.isA().page.isA(); + }); + + testWidgets('do not push route to non-empty navigator stack', (tester) async { + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + + // Set up long enough loading time to later navigate to the choose-account + // page from the loading page via the "Try another account" button. + const loadPerAccountDuration = Duration(seconds: 30); + assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod); + testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration; + testBinding.globalStore.loadPerAccountException = eg.apiExceptionUnauthorized(); + await prepare(tester); + // The navigator stack should contain only a home page route. + + await tester.pump(kTryAnotherAccountWaitPeriod); + await tester.tap(find.text('Try another account')); + await tester.pump(); // tap the button + // The navigator stack should contain the home page route + // and a choose-account page route. + check(removedRoutes).isEmpty(); + check(poppedRoutes).isEmpty(); + check(pushedRoutes).single.isA().page.isA(); + pushedRoutes.clear(); + + await tester.pump(loadPerAccountDuration); // got the error + await tester.pump(TestGlobalStore.removeAccountDuration); + // The navigator stack should contain the choose-account page route + // and a dialog route. + // The home page route was removed because of account logout. + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + check(removedRoutes).single.isA().page.isA(); + check(poppedRoutes).isEmpty(); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Could not connect', + expectedMessage: + 'Your account at ${eg.selfAccount.realmUrl} could not be authenticated.' + ' Please try logging in again or use another account.'))); + // The navigator stack should contain only the choose-account page route. + // No routes should be pushed after dismissing the error dialog, + // because the navigator stack was non-empty. + check(poppedRoutes).single.isA>(); + check(pushedRoutes).isEmpty(); + }); }); group('ChooseAccountPage', () {