store: Handle invalid API key on register-queue

PIG208 · PIG208 · commit a62252d9de3f · 2025-02-13T23:04:27.000-05:00
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 zulip#890 by handling authentication errors for register-queue. Fixes: zulip#737 Signed-off-by: Zixuan James Li <zixuan@zulip.com>
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<PerAccountStore> 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
@@ -7,13 +7,14 @@ import 'package:flutter/foundation.dart';
 import 'package:http/http.dart' as http;
 import 'package:test/scaffolding.dart';
 import 'package:zulip/api/core.dart';
+import 'package:zulip/api/exception.dart';
 import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/events.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/api/route/realm.dart';
-import 'package:zulip/log.dart';
 import 'package:zulip/model/actions.dart';
+import 'package:zulip/log.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/notifications/receive.dart';
 
@@ -121,6 +122,39 @@ void main() {
     check(completers(1)).length.equals(1);
   });
 
+  test('GlobalStore.perAccount loading fails with HTTP status code 401', () => awaitFakeAsync((async) async {
+    addTearDown(testBinding.reset);
+
+    await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
+    testBinding.globalStore.loadPerAccountException = ZulipApiException(
+      routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401,
+      data: {}, message: '');
+    await check(testBinding.globalStore.perAccount(eg.selfAccount.id))
+      .throws<AccountNotFoundException>();
+
+    check(testBinding.globalStore.takeDoRemoveAccountCalls())
+      .single.equals(eg.selfAccount.id);
+  }));
+
+  test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async {
+    addTearDown(testBinding.reset);
+
+    await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
+
+    testBinding.globalStore.loadPerAccountDuration = Duration(seconds: 2);
+    testBinding.globalStore.loadPerAccountException = ZulipApiException(
+      routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401,
+      data: {}, message: '');
+    final future = testBinding.globalStore.perAccount(eg.selfAccount.id);
+    check(testBinding.globalStore.takeDoRemoveAccountCalls()).isEmpty();
+
+    await logOutAccount(testBinding.globalStore, eg.selfAccount.id);
+    check(testBinding.globalStore.takeDoRemoveAccountCalls()).single;
+
+    await check(future).throws<AccountNotFoundException>();
+    check(testBinding.globalStore.takeDoRemoveAccountCalls()).isEmpty();
+  }));
+
   // TODO test insertAccount
 
   group('GlobalStore.updateAccount', () {
@@ -975,7 +1009,7 @@ void main() {
     group('errors while reloading', () {
       void checkReloadFailure({
         required void Function() prepareError,
-        required Future<void> Function(FakeAsync async) callbackWhileReloading,
+        required FutureOr<void> Function(FakeAsync async) callbackWhileReloading,
       }) {
         awaitFakeAsync((async) async {
           void complete() => (globalStore as LoadingTestGlobalStore)
@@ -1017,11 +1051,23 @@ void main() {
         await future;
       }
 
+      void prepareLoadPerAccountZulipApiExceptionUnauthorized(FakeAsync async) {
+        globalStore.loadPerAccountException = ZulipApiException(
+          routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401,
+          data: {}, message: '');
+      }
+
       test('user logged out', () => awaitFakeAsync((async) async {
         checkReloadFailure(
           prepareError: prepareExpiredEventQueue,
           callbackWhileReloading: doLogOutAccount);
       }));
+
+      test('got status 401 ZulipApiException', () => awaitFakeAsync((async) async {
+        checkReloadFailure(
+          prepareError: prepareUnexpectedLoopError,
+          callbackWhileReloading: prepareLoadPerAccountZulipApiExceptionUnauthorized);
+      }));
     });
   });
 
diff --git 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<int> takeDoRemoveAccountCalls() {
@@ -150,6 +151,9 @@ class TestGlobalStore extends GlobalStore {
     if (loadPerAccountDuration != null) {
       await Future<void>.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
@@ -3,6 +3,7 @@ import 'dart:async';
 import 'package:checks/checks.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter_test/flutter_test.dart';
+import 'package:zulip/api/exception.dart';
 import 'package:zulip/log.dart';
 import 'package:zulip/model/actions.dart';
 import 'package:zulip/model/database.dart';
@@ -61,22 +62,89 @@ void main() {
   group('_PreventEmptyStack', () {
     late List<Route<void>> pushedRoutes;
     late List<Route<void>> removedRoutes;
+    late List<Route<void>> poppedRoutes;
 
     Future<void> 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
       check(pushedRoutes).single.isA<WidgetRoute>().page.isA<HomePage>();
       pushedRoutes.clear();
     }
 
+    testWidgets('do not push route to non-empty navigator stack', (tester) async {
+      const loadPerAccountDuration = Duration(seconds: 30);
+      assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod);
+      testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration;
+      testBinding.globalStore.loadPerAccountException = ZulipApiException(
+        routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401,
+        data: {}, message: '');
+      await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
+      await prepare(tester);
+
+      await tester.pump(kTryAnotherAccountWaitPeriod);
+      await tester.tap(find.text('Try another account'));
+      await tester.pump(); // tap the button
+      check(pushedRoutes).single.isA<WidgetRoute>().page.isA<ChooseAccountPage>();
+      pushedRoutes.clear();
+
+      await tester.pump(loadPerAccountDuration); // got the error
+      await tester.pump(TestGlobalStore.removeAccountDuration);
+      check(testBinding.globalStore.takeDoRemoveAccountCalls())
+        .single.equals(eg.selfAccount.id);
+      check(removedRoutes).single.isA<WidgetRoute>().page.isA<HomePage>();
+      check(poppedRoutes).isEmpty();
+      check(pushedRoutes).single.isA<DialogRoute<void>>();
+      pushedRoutes.clear();
+
+      await tester.tap(find.byWidget(checkErrorDialog(tester,
+        expectedTitle: 'Could not connect',
+        expectedMessage:
+          'Your account at https://chat.example/ could not be authenticated.'
+          ' Please try logging in again or use another account.')));
+      // No more routes are pushed after dismissing the error dialog,
+      // because the navigator stack was non-empty.
+      check(poppedRoutes).single.isA<DialogRoute<void>>();
+      check(pushedRoutes).isEmpty();
+    });
+
+    testWidgets('push route when popping last route on stack', (tester) async {
+      testBinding.globalStore.loadPerAccountDuration = Duration.zero;
+      testBinding.globalStore.loadPerAccountException = ZulipApiException(
+        routeName: '/register', code: 'UNAUTHORIZED', httpStatus: 401,
+        data: {}, message: '');
+      await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
+      await prepare(tester);
+
+      await tester.pump(Duration.zero); // got the error
+      await tester.pump(TestGlobalStore.removeAccountDuration);
+      check(testBinding.globalStore.takeDoRemoveAccountCalls())
+        .single.equals(eg.selfAccount.id);
+      check(removedRoutes).single.isA<WidgetRoute>().page.isA<HomePage>();
+      check(poppedRoutes).isEmpty();
+      check(pushedRoutes).single.isA<DialogRoute<void>>();
+      pushedRoutes.clear();
+
+      await tester.tap(find.byWidget(checkErrorDialog(tester,
+        expectedTitle: 'Could not connect',
+        expectedMessage:
+          'Your account at https://chat.example/ could not be authenticated.'
+          ' Please try logging in again or use another account.')));
+      // The navigator stack became empty after dismissing the error dialog,
+      // so a choose-account page route was pushed.
+      check(poppedRoutes).single.isA<DialogRoute<void>>();
+      check(pushedRoutes).single.isA<WidgetRoute>().page.isA<ChooseAccountPage>();
+    });
+
     testWidgets('push route when removing last route on stack', (tester) async {
       await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
       await prepare(tester);
