store: Handle invalid API key on register-queue

PIG208 · PIG208 · commit a43e320dfe87 · 2024-12-20T19:38:31.000-05:00
This is near term fix for a user-reported issue: https://chat.zulip.org/#narrow/channel/48-mobile/topic/0.2E0.2E19.20Flutter.20.3A.20Cant.20connect.20to.20self.20hosted.20instance/near/2004042 It is not intended to be the full fix. With a better UX, we would bring the user back to the choose-account page without them manually doing so. That's covered by #737 but out-of-scope for this commit. Signed-off-by: Zixuan James Li <zixuan@zulip.com>
diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb
@@ -186,8 +186,8 @@
       "url": {"type": "String", "example": "http://example.com/"}
     }
   },
-  "errorLoginCouldNotConnectTitle": "Could not connect",
-  "@errorLoginCouldNotConnectTitle": {
+  "errorCouldNotConnectTitle": "Could not connect",
+  "@errorCouldNotConnectTitle": {
     "description": "Error title when the app could not connect to the server."
   },
   "errorMessageDoesNotSeemToExist": "That message does not seem to exist.",
@@ -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 logout and try again.",
+  "@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
@@ -359,7 +359,7 @@ abstract class ZulipLocalizations {
   ///
   /// In en, this message translates to:
   /// **'Could not connect'**
-  String get errorLoginCouldNotConnectTitle;
+  String get errorCouldNotConnectTitle;
 
   /// Error message when loading a message that does not exist.
   ///
@@ -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 logout and try again.'**
+  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
@@ -159,7 +159,7 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
   }
 
   @override
-  String get errorLoginCouldNotConnectTitle => 'Could not connect';
+  String get errorCouldNotConnectTitle => 'Could not connect';
 
   @override
   String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -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 logout and try again.';
+  }
+
   @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
@@ -159,7 +159,7 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
   }
 
   @override
-  String get errorLoginCouldNotConnectTitle => 'Could not connect';
+  String get errorCouldNotConnectTitle => 'Could not connect';
 
   @override
   String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -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 logout and try again.';
+  }
+
   @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
@@ -159,7 +159,7 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
   }
 
   @override
-  String get errorLoginCouldNotConnectTitle => 'Could not connect';
+  String get errorCouldNotConnectTitle => 'Could not connect';
 
   @override
   String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -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 logout and try again.';
+  }
+
   @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
@@ -159,7 +159,7 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
   }
 
   @override
-  String get errorLoginCouldNotConnectTitle => 'Could not connect';
+  String get errorCouldNotConnectTitle => 'Could not connect';
 
   @override
   String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -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 logout and try again.';
+  }
+
   @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
@@ -159,7 +159,7 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
   }
 
   @override
-  String get errorLoginCouldNotConnectTitle => 'Nie można połączyć';
+  String get errorCouldNotConnectTitle => 'Could not connect';
 
   @override
   String get errorMessageDoesNotSeemToExist => 'Taka wiadomość raczej nie istnieje.';
@@ -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 logout and try again.';
+  }
+
   @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
@@ -159,7 +159,7 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
   }
 
   @override
-  String get errorLoginCouldNotConnectTitle => 'Could not connect';
+  String get errorCouldNotConnectTitle => 'Could not connect';
 
   @override
   String get errorMessageDoesNotSeemToExist => 'That message does not seem to exist.';
@@ -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 logout and try again.';
+  }
+
   @override
   String get errorInvalidResponse => 'The server sent an invalid response';
 
diff --git a/lib/log.dart b/lib/log.dart
@@ -31,7 +31,8 @@ bool debugLog(String message) {
   return true;
 }
 
-typedef ReportErrorCallback = void Function(String? message, {String? details});
+typedef ReportErrorCancellablyCallback = void Function(String? message, {String? details});
+typedef ReportErrorCallback = void Function(String message, {String? details});
 
 /// Show the user an error message, without requiring them to interact with it.
 ///
@@ -48,7 +49,18 @@ typedef ReportErrorCallback = void Function(String? message, {String? details});
 // This gets set in [ZulipApp].  We need this indirection to keep `lib/log.dart`
 // from importing widget code, because the file is a dependency for the rest of
 // the app.
-ReportErrorCallback reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
+ReportErrorCancellablyCallback reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
+
+/// Show the user a dismissable error message in a modal popup.
+///
+/// Typically this shows a [AlertDialog] containing the message.
+/// If called before the app's widget tree is ready (see [ZulipApp.ready]),
+/// then we give up on showing the message to the user,
+/// and just log the message to the console.
+// This gets set in [ZulipApp].  We need this indirection to keep `lib/log.dart`
+// from importing widget code, because the file is a dependency for the rest of
+// the app.
+ReportErrorCallback reportErrorToUserModally = defaultReportErrorToUserBriefly;
 
 void defaultReportErrorToUserBriefly(String? message, {String? details}) {
   // Error dismissing is a no-op to the default handler.
diff --git a/lib/model/store.dart b/lib/model/store.dart
@@ -912,7 +912,16 @@ 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'):
+            final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
+            reportErrorToUserModally(
+              zulipLocalizations.errorCouldNotConnectTitle,
+              details: zulipLocalizations.errorInvalidApiKeyMessage(
+                connection.realmUrl.toString()));
+            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
@@ -85,6 +85,7 @@ class ZulipApp extends StatefulWidget {
   static void debugReset() {
     _snackBarCount = 0;
     reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
+    reportErrorToUserModally = defaultReportErrorToUserBriefly;
     _ready.dispose();
     _ready = ValueNotifier(false);
   }
@@ -128,10 +129,21 @@ class ZulipApp extends StatefulWidget {
     newSnackBar.closed.whenComplete(() => _snackBarCount--);
   }
 
+  /// The callback we normally use as [reportErrorToUserModally].
+  static void _reportErrorToUserModally(String message, {String? details}) {
+    assert(_ready.value);
+
+    showErrorDialog(
+      context: navigatorKey.currentContext!,
+      title: message,
+      message: details);
+  }
+
   void _declareReady() {
     assert(navigatorKey.currentContext != null);
     _ready.value = true;
     reportErrorToUserBriefly = _reportErrorToUserBriefly;
+    reportErrorToUserModally = _reportErrorToUserModally;
   }
 
   @override
diff --git a/lib/widgets/login.dart b/lib/widgets/login.dart
@@ -176,7 +176,7 @@ class _AddAccountPageState extends State<AddAccountPage> {
         // TODO(#105) give more helpful feedback; see `fetchServerSettings`
         //   in zulip-mobile's src/message/fetchActions.js.
         showErrorDialog(context: context,
-          title: zulipLocalizations.errorLoginCouldNotConnectTitle,
+          title: zulipLocalizations.errorCouldNotConnectTitle,
           message: zulipLocalizations.errorLoginCouldNotConnect(url.toString()));
         return;
       }
diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart
@@ -1,6 +1,7 @@
 import 'package:flutter/material.dart';
 import 'package:flutter/scheduler.dart';
 
+import '../api/exception.dart';
 import '../model/binding.dart';
 import '../model/store.dart';
 import 'page.dart';
@@ -245,11 +246,23 @@ 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.
+              // TODO(#737): Reset the navigator stack and bring the user back
+              //   to the choose-account page.
+              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,36 @@ void main() {
         users.map((expected) => (it) => it.fullName.equals(expected.fullName)));
     }));
 
+    String? lastReportedError;
+    String? takeLastReportedError() {
+      final result = lastReportedError;
+      lastReportedError = null;
+      return result;
+    }
+
+    Future<void> logReportedError(String message, {String? details}) async {
+      lastReportedError = '$message\n$details';
+    }
+
+    test('does not retry registerQueue on invalid API key', () => awaitFakeAsync((async) async {
+      reportErrorToUserModally = logReportedError;
+      addTearDown(() => reportErrorToUserModally = defaultReportErrorToUserBriefly);
+      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>();
+
+      check(takeLastReportedError()).isNotNull().equals(
+        'Invalid API key\n'
+        'Your account at https://chat.example/ cannot be authenticated. Please logout and try again.');
+    }));
+
     // TODO test UpdateMachine.load starts polling loop
     // TODO test UpdateMachine.load calls registerNotificationToken
   });
diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart
@@ -2,6 +2,7 @@ import 'dart:async';
 
 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/log.dart';
 import 'package:zulip/model/database.dart';
@@ -361,5 +362,24 @@ void main() {
       await tester.pumpAndSettle();
       check(findSnackBarByText('unrelated').evaluate()).single;
     });
+
+    testWidgets('reportErrorToUserModally', (tester) async {
+      addTearDown(testBinding.reset);
+      await tester.pumpWidget(const ZulipApp());
+      const message = 'test error message';
+      const details = 'details';
+
+      // Prior to app startup, reportErrorToUserModally only logs.
+      reportErrorToUserModally(message, details: details);
+      check(ZulipApp.ready).value.isFalse();
+      await tester.pump();
+      check(find.byType(AlertDialog)).findsNothing();
+
+      check(ZulipApp.ready).value.isTrue();
+      // After app startup, reportErrorToUserModally displays an [AlertDialog].
+      reportErrorToUserModally(message, details: details);
+      await tester.pump();
+      checkErrorDialog(tester, expectedTitle: message, expectedMessage: details);
+    });
   });
 }
