store: Handle invalid API key on register-queue

PIG208 · PIG208 · commit 31020b6d2291 · 2025-01-03T10:23:31.000+08:00
This fixes the user reported issue by navigating them back to the choose-account page if the API key is found invalid: https://chat.zulip.org/#narrow/channel/48-mobile/topic/0.2E0.2E19.20Flutter.20.3A.20Cant.20connect.20to.20self.20hosted.20instance/near/2004042 A follow-up would be bringing the user back to the login page (so that the realm url is pre-filled). Signed-off-by: Zixuan James Li <zixuan@zulip.com>
diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb
@@ -458,6 +458,13 @@
   "@topicValidationErrorMandatoryButEmpty": {
     "description": "Topic validation error when topic is required but was empty."
   },
+  "errorInvalidApiKeyMessage": "Your account at {url} cannot be authenticated. Please try 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
@@ -721,6 +721,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} cannot be authenticated. Please try 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
@@ -357,6 +357,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 cannot be authenticated. Please try 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
@@ -357,6 +357,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 cannot be authenticated. Please try again or use another account.';
+  }
+
   @override
   String get errorInvalidResponse => 'The server sent an invalid response';
 
diff --git a/lib/generated/l10n/zulip_localizations_fr.dart b/lib/generated/l10n/zulip_localizations_fr.dart
@@ -357,6 +357,11 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
   @override
   String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
 
+  @override
+  String errorInvalidApiKeyMessage(String url) {
+    return 'Your account at $url cannot be authenticated. Please try 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
@@ -357,6 +357,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 cannot be authenticated. Please try 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
@@ -357,6 +357,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 cannot be authenticated. Please try 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
@@ -357,6 +357,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
   @override
   String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
 
+  @override
+  String errorInvalidApiKeyMessage(String url) {
+    return 'Your account at $url cannot be authenticated. Please try again or use another account.';
+  }
+
   @override
   String get errorInvalidResponse => 'The server sent an invalid response';
 
diff --git a/lib/model/store.dart b/lib/model/store.dart
@@ -912,7 +912,12 @@ class UpdateMachine {
         // 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 tell user if initial-fetch errors persist, or look non-transient
+        // TODO(#890): tell user if initial-fetch errors persist, or look non-transient
+        switch (e) {
+          case ZulipApiException(code: 'INVALID_API_KEY'):
+            // See also: [_PerAccountStoreWidgetState.didChangeDependencies]
+            rethrow;
+        }
         await (backoffMachine ??= BackoffMachine()).wait();
         assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
       }
diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart
@@ -233,6 +233,14 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
 class ChooseAccountPage extends StatelessWidget {
   const ChooseAccountPage({super.key});
 
+  /// Navigate to [ChooseAccountPage], ensuring that its route is at the root level.
+  static void navigate(BuildContext context) {
+    final navigator = Navigator.of(context);
+    navigator.popUntil((route) => route.isFirst);
+    unawaited(navigator.pushReplacement(
+      MaterialWidgetRoute(page: const ChooseAccountPage())));
+  }
+
   Widget _buildAccountItem(
     BuildContext context, {
     required int accountId,
diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart
@@ -1,8 +1,15 @@
+import 'dart:async';
+
 import 'package:flutter/material.dart';
 import 'package:flutter/scheduler.dart';
 
+import '../api/exception.dart';
+import '../generated/l10n/zulip_localizations.dart';
+import '../log.dart';
 import '../model/binding.dart';
 import '../model/store.dart';
+import 'actions.dart';
+import 'app.dart';
 import 'page.dart';
 
 /// Provides access to the app's data.
@@ -228,7 +235,16 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
       _setStore(null);
       if (widget.routeToRemoveOnLogout != null) {
         SchedulerBinding.instance.addPostFrameCallback(
-          (_) => Navigator.of(context).removeRoute(widget.routeToRemoveOnLogout!));
+          (_) {
+            final navigator = Navigator.of(context);
+            navigator.removeRoute(widget.routeToRemoveOnLogout!);
+            if (!navigator.canPop()) {
+              // This ensures that the navigator stack is non-empty after the
+              // removal of the route.
+              unawaited(navigator.push(
+                MaterialWidgetRoute(page: const ChooseAccountPage())));
+            }
+          });
       }
       return;
     }
@@ -245,11 +261,28 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
           // [didChangeDependencies] will run again, this time in the
           // `store != null` case above.
           await globalStore.perAccount(widget.accountId);
-        } on AccountNotFoundException {
-          // The account was logged out while its store was loading.
-          // This widget will be showing [placeholder] perpetually,
-          // but that's OK as long as other code will be removing it from the UI
-          // (usually by using [routeToRemoveOnLogout]).
+        } catch (e) {
+          switch (e) {
+            case AccountNotFoundException():
+              // The account was logged out while its store was loading.
+              // This widget will be showing [placeholder] perpetually,
+              // but that's OK as long as other code will be removing it from the UI
+              // (usually by using [routeToRemoveOnLogout]).
+              return;
+            case ZulipApiException(code: 'INVALID_API_KEY'):
+              // The API key is invalid and the store can never be loaded
+              // unless the user retries manually.
+              if (!mounted) return;
+              final zulipLocalizations = ZulipLocalizations.of(context);
+              reportErrorToUserModally(
+                zulipLocalizations.errorCouldNotConnectTitle,
+                details: zulipLocalizations.errorInvalidApiKeyMessage(
+                  globalStore.getAccount(widget.accountId)!.realmUrl.toString()));
+              unawaited(logOutAccount(context, widget.accountId));
+              return;
+            default:
+              rethrow;
+          }
         }
       }();
     }
diff --git a/test/model/store_test.dart b/test/model/store_test.dart
@@ -7,6 +7,7 @@ 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';
@@ -500,6 +501,19 @@ void main() {
         users.map((expected) => (it) => it.fullName.equals(expected.fullName)));
     }));
 
+    test('does not retry registerQueue on invalid API key', () => awaitFakeAsync((async) async {
+      await prepareStore();
+
+      // Try to load, but the API key is told to be invalid.
+      globalStore.useCachedApiConnections = true;
+      connection.prepare(httpStatus: 400, json: {
+        'result': 'error', 'code': 'INVALID_API_KEY',
+        'msg': 'Invalid API key',
+      });
+      await check(UpdateMachine.load(globalStore, eg.selfAccount.id))
+        .throws<ZulipApiException>();
+    }));
+
     // TODO test UpdateMachine.load starts polling loop
     // TODO test UpdateMachine.load calls registerNotificationToken
   });
diff --git a/test/model/test_store.dart b/test/model/test_store.dart
@@ -126,6 +126,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() {
@@ -147,6 +148,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/store_test.dart b/test/widgets/store_test.dart
@@ -1,13 +1,19 @@
 import 'package:checks/checks.dart';
 import 'package:flutter/material.dart';
+import 'package:flutter_checks/flutter_checks.dart';
 import 'package:flutter_test/flutter_test.dart';
+import 'package:zulip/api/exception.dart';
 import 'package:zulip/model/store.dart';
+import 'package:zulip/widgets/app.dart';
+import 'package:zulip/widgets/home.dart';
 import 'package:zulip/widgets/store.dart';
 
 import '../flutter_checks.dart';
 import '../model/binding.dart';
 import '../example_data.dart' as eg;
 import '../model/store_checks.dart';
+import '../model/test_store.dart';
+import 'dialog_checks.dart';
 
 /// A widget whose state uses [PerAccountStoreAwareStateMixin].
 class MyWidgetWithMixin extends StatefulWidget {
@@ -113,6 +119,45 @@ void main() {
       .contains('consider MaterialAccountWidgetRoute');
   });
 
+  testWidgets('PerAccountStoreWidget reset to choose-account page'
+              ' when logging out on invalid API key', (tester) async {
+    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
+    addTearDown(testBinding.reset);
+
+    const loadPerAccountDuration = Duration(seconds: 30);
+    assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod);
+    testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration;
+    testBinding.globalStore.loadPerAccountException = ZulipApiException(
+      routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400,
+      data: {}, message: '');
+    await tester.pumpWidget(const ZulipApp());
+    await tester.pump(); // start to load account
+    check(find.byType(CircularProgressIndicator)).findsOne();
+    check(find.byType(ChooseAccountPage)).findsNothing();
+    check(find.byType(BackButton)).findsNothing();
+
+    await tester.pump(kTryAnotherAccountWaitPeriod);
+    await tester.tap(find.text('Try another account'));
+    await tester.pump(); // tap the button
+    await tester.pump(const Duration(milliseconds: 250)); // wait for animation
+    check(find.byType(CircularProgressIndicator)).findsOne();
+    check(find.byType(ChooseAccountPage)).findsOne();
+    check(find.byType(BackButton)).findsOne();
+
+    await tester.pump(loadPerAccountDuration);
+    await tester.pump(TestGlobalStore.removeAccountDuration);
+    await tester.pump(const Duration(milliseconds: 250)); // wait for animation
+    check(find.byType(CircularProgressIndicator)).findsNothing();
+    check(find.byType(ChooseAccountPage)).findsOne();
+    // Choose-account page's route should be at the root level.
+    check(find.byType(BackButton)).findsNothing();
+    checkErrorDialog(tester,
+      expectedTitle: 'Could not connect',
+      expectedMessage:
+        'Your account at https://chat.example/ cannot be authenticated.'
+        ' Please try again or use another account.');
+  });
+
   testWidgets('PerAccountStoreWidget immediate data after first loaded', (tester) async {
     await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
     addTearDown(testBinding.reset);
