Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store: Handle invalid API key on register-queue #1183

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
11 changes: 9 additions & 2 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,8 @@
"url": {"type": "String", "example": "http://example.com/"}
}
},
"errorLoginCouldNotConnectTitle": "Could not connect",
"@errorLoginCouldNotConnectTitle": {
"errorCouldNotConnectTitle": "Could not connect",
"@errorCouldNotConnectTitle": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l10n [nfc]: Use a generalize name for errorCouldNotConnectTitle

commit-message nit: "generalized"

"description": "Error title when the app could not connect to the server."
},
"errorMessageDoesNotSeemToExist": "That message does not seem to exist.",
Expand Down Expand Up @@ -458,6 +458,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."
Expand Down
8 changes: 7 additions & 1 deletion lib/generated/l10n/zulip_localizations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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} 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:
Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_ar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -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 could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -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 could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_fr.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -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 could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_ja.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -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 could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_pl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -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 could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera';

Expand Down
7 changes: 6 additions & 1 deletion lib/generated/l10n/zulip_localizations_ru.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -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 could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
18 changes: 15 additions & 3 deletions lib/log.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -48,9 +49,20 @@ 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 = reportErrorToConsole;

void defaultReportErrorToUserBriefly(String? message, {String? details}) {
/// 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 = reportErrorToConsole;

void reportErrorToConsole(String? message, {String? details}) {
// Error dismissing is a no-op to the default handler.
if (message == null) return;
// If this callback is still in place, then the app's widget tree
Expand Down
43 changes: 37 additions & 6 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import '../api/backoff.dart';
import '../api/route/realm.dart';
import '../log.dart';
import '../notifications/receive.dart';
import '../widgets/actions.dart';
import 'autocomplete.dart';
import 'database.dart';
import 'emoji.dart';
Expand Down Expand Up @@ -149,13 +150,31 @@ abstract class GlobalStore extends ChangeNotifier {
/// and/or [perAccountSync].
Future<PerAccountStore> loadPerAccount(int accountId) async {
assert(_accounts.containsKey(accountId));
final store = await doLoadPerAccount(accountId);
PerAccountStore? store;
try {
store = await doLoadPerAccount(accountId);
} catch (e) {
switch (e) {
case ZulipApiException(code: 'INVALID_API_KEY'):
// The API key is invalid and the store can never be loaded
// unless the user retries manually.
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
final account = getAccount(accountId)!;
reportErrorToUserModally(
zulipLocalizations.errorCouldNotConnectTitle,
details: zulipLocalizations.errorInvalidApiKeyMessage(
account.realmUrl.toString()));
await logOutAccount(this, accountId);
default:
rethrow;
}
}
if (!_accounts.containsKey(accountId)) {
// [removeAccount] was called during [doLoadPerAccount].
store.dispose();
// [removeAccount] was called during or after [doLoadPerAccount].
store?.dispose();
throw AccountNotFoundException();
}
return store;
return store!;
}

/// Load per-account data for the given account, unconditionally.
Expand Down Expand Up @@ -912,7 +931,13 @@ 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'):
// We cannot recover from this error through retrying.
// Leave it to [GlobalStore.loadPerAccount].
rethrow;
}
await (backoffMachine ??= BackoffMachine()).wait();
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
}
Expand Down Expand Up @@ -1044,7 +1069,13 @@ class UpdateMachine {
}
} catch (e) {
if (_disposed) return;
await _handlePollError(e);
try {
await _handlePollError(e);
} on AccountNotFoundException {
// Cannot recover by replacing the store because the account
// was logged out.
return;
}
assert(_disposed);
return;
}
Expand Down
4 changes: 1 addition & 3 deletions lib/widgets/actions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ import '../notifications/receive.dart';
import 'dialog.dart';
import 'store.dart';

Future<void> logOutAccount(BuildContext context, int accountId) async {
final globalStore = GlobalStoreWidget.of(context);

Future<void> logOutAccount(GlobalStore globalStore, int accountId) async {
final account = globalStore.getAccount(accountId);
if (account == null) return; // TODO(log)

Expand Down
53 changes: 50 additions & 3 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ class ZulipApp extends StatefulWidget {
@visibleForTesting
static void debugReset() {
_snackBarCount = 0;
reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
reportErrorToUserBriefly = reportErrorToConsole;
reportErrorToUserModally = reportErrorToConsole;
_ready.dispose();
_ready = ValueNotifier(false);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -177,6 +189,11 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
final themeData = zulipThemeData(context);
return GlobalStoreWidget(
child: Builder(builder: (context) {
final effectiveNavigatorObservers = [
_EmptyStackNavigatorObserver(),
if (widget.navigatorObservers != null)
...widget.navigatorObservers!,
];
final globalStore = GlobalStoreWidget.of(context);
// TODO(#524) choose initial account as last one used
final initialAccountId = globalStore.accounts.firstOrNull?.id;
Expand All @@ -187,7 +204,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
theme: themeData,

navigatorKey: ZulipApp.navigatorKey,
navigatorObservers: widget.navigatorObservers ?? const [],
navigatorObservers: effectiveNavigatorObservers,
builder: (BuildContext context, Widget? child) {
if (!ZulipApp.ready.value) {
SchedulerBinding.instance.addPostFrameCallback(
Expand Down Expand Up @@ -218,6 +235,36 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
}
}

class _EmptyStackNavigatorObserver extends NavigatorObserver {
void _pushIfEmpty() async {
final navigator = await ZulipApp.navigator;
bool isEmptyStack = true;
// TODO: find a better way to inspect the navigator stack
navigator.popUntil((route) {
isEmptyStack = false;
return true; // never actually pops
});
if (isEmptyStack) {
unawaited(navigator.push(
MaterialWidgetRoute(page: const ChooseAccountPage())));
}
}

@override
void didRemove(Route<void> route, Route<void>? previousRoute) async {
if (previousRoute == null) {
_pushIfEmpty();
}
}

@override
void didPop(Route<void> route, Route<void>? previousRoute) async {
if (previousRoute == null) {
_pushIfEmpty();
}
}
}

class ChooseAccountPage extends StatelessWidget {
const ChooseAccountPage({super.key});

Expand Down Expand Up @@ -249,7 +296,7 @@ class ChooseAccountPage extends StatelessWidget {
actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton,
onActionButtonPress: () {
// TODO error handling if db write fails?
logOutAccount(context, accountId);
logOutAccount(GlobalStoreWidget.of(context), accountId);
});
},
child: Text(zulipLocalizations.chooseAccountPageLogOutButton)),
Expand Down
Loading